Skip to content

Conversation

@lispwitch
Copy link

@lispwitch lispwitch commented May 13, 2018

Test BigInt serialization in IndexedDB values, and check that BigInt values cannot be used as IndexedDB keys. These are @littledan's tests with some changes suggested in reviews; see PR #9667 for the original version.

BigInt and BigInt wrappers are supported in serialization, per
whatwg/html#3480
This support allows them to be used as IndexedDB values.

However, BigInt is not supported as an IndexedDB key; support
has been proposed in the following PR, but that change has not
landed at the time this patch was written
w3c/IndexedDB#231
@lispwitch
Copy link
Author

cc @littledan @inexorabletash

Copy link
Contributor

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the submission!


function invalidKey(key, name) {
async_test(t => {
createdb(t).onupgradeneeded = t.step_func(e => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier to just use e.g. indexedDB.cmp(0, key) - this will throw if the key is not valid. No need to create a database.

async_test(t => {
t.step(function() {
assert_true(test(value),
"condition does not apply to initial value");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertion messages should be phrased in the form of the expected behavior, e.g. "Predicate should return true for the initial value"; the error output is confusing if the inverse is used.

// This support allows them to be used as IndexedDB values.

let i = 0;
function value(value, test, name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename the test argument to test_func or predicate? Since test is a testharness function it's confusing to see it re-used as an argument.

// This support allows them to be used as IndexedDB values.

let i = 0;
function value(value, test, name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using value as both the name of the function and the argument is confusing. How about value_test for the function?

.createObjectStore("store")
.add(value, 1);

e.target.onsuccess = t.step_func(function(e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrow function instead of function ?

.transaction("store")
.objectStore("store")
.get(1)
.onsuccess = t.step_func(function(e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrow function instead of function ?

@@ -0,0 +1,77 @@
<!DOCTYPE html>
<meta charset="utf-8">
<title>Values</title>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide a more informative title

<script src="support.js"></script>

<script>
// BigInt and BigInt wrappers are supported in serialization, per
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "objects" instead of "wrappers" to be consistent with the usage below? Or use "wrappers" everywhere.

// https://github.com/whatwg/html/pull/3480
// This support allows them to be used as IndexedDB values.

let i = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

@lispwitch
Copy link
Author

implemented the requested changes, and added a missing .valueOf() call

Copy link
Contributor

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@inexorabletash inexorabletash merged commit b2e3e49 into web-platform-tests:master May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants