-
-
Notifications
You must be signed in to change notification settings - Fork 400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed conformance reporting #3348
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3348 +/- ##
==========================================
- Coverage 47.24% 45.42% -1.82%
==========================================
Files 476 483 +7
Lines 46892 49768 +2876
==========================================
+ Hits 22154 22609 +455
- Misses 24738 27159 +2421 ☔ View full report in Codecov by Sentry. |
boa_tester/src/main.rs
Outdated
strict: Option<TestOutcomeResult>, | ||
#[serde(rename = "r", default)] | ||
no_strict: Option<TestOutcomeResult>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could simplify this by making TestOutcomeResult
something like:
#[serde(tag = "t", content = "d")
enum TestOutcomeResult {
#[serde(rename = "O")]
Passed,
#[serde(rename = "I")]
Ignored,
#[serde(rename = "F")]
Failed {
strict: bool,
reason: String
},
#[serde(rename = "P")]
Panic {
strict: bool,
reason: String,
}
}
Then, we won't need separate fields for strict
and no_strict
, just a single result
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that, and also about the file size & JS performance in the browser. I think that having dictionaries with test/suite names as keys would decrease the size of the JSON around a 15%.
Booleans occupy a bit more. Maybe adding more variants, such as "StrictFailed" and so on could be more performant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's time to transition to a binary serialization format such as MessagePack?
Another idea I had was to use Postcard and a small wasm glue code to deserialize the postcard and serialize it to JSON, then we would call that function from the conformance page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll play a bit with those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've given this a thought, and it could be a nice enhancement in the future, but I would for now just use JSON until the new website is up and running. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we can revisit this when we finish the site redesign.
Putting that aside, I still think it would be better to have FailedStrict
and PanicStrict
for simplicity, if we do use that info. If we don't, maybe we can obviate the need for those variants and just have a single result
field?
I converted it back to draft, because I'm still missing:
|
391416e
to
b68e588
Compare
Since we will probably do a release before this is finished, and we need to fix our tester before then, I'll open a PR with a simpler version of this that just fixes the bug without changing the data representation too much. We can apply the changes of this PR after that. |
0ba1929
to
3a8360b
Compare
This has been rebased. It's still missing:
|
We received reports in our Discord saying that our conformance representation was wrong.
We count a test that needs to be run both in strict and non-strict mode as 2 tests, and if it passes in both modes, we count 2 tests as passing, while if only one of them passes, we count one as passed, one as failed.
The correct interpretation is that a test is only considered passing if both strict and no-strict modes pass (tc39/test262#3871).
It changes the following:
TestResult
now contains 2 fields:strict
andno_strict
.strict
is serialized ass
, andno_strict
is serialized asr
(from "result")TestResult
structure, since it was being skipped and never usedThis requires a change in the conformance page to adhere to the new result format, and we need to migrate the current conformance results. I'll open a new PR for that.
This PR will fail when doing the result comparison, thoguh, so we might need to first merge the new result format, then this.