fix(user-timing): fix clearing marks/measures by name #32120
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nolanlawson
commented
Apr 20, 2024
| .as_ref() | ||
| .map_or(true, |type_| *e.entry_type() != *type_) | ||
| *e.entry_type() != *entry_type || | ||
| name.as_ref().map_or(false, |name_| *e.name() != *name_) |
There was a problem hiding this comment.
When the name is None, then the expectation is that all marks/measures are removed. If it's Some, then we need to compare the name and only remove entries that match.
|
Ah, I suspect that that wpt test directory is not part of the default set of enabled directories. We should update the test config to correct that! |
|
Ah, I see, so should I could do that in this PR or a separate one if you'd like. |
|
Why don't we make a separate PR that changes that file and adds the current set if expected results, then this PR can show the change in behavior more clearly? |
|
Alternatively, doing that work in this PR in two separate commits would also be ok. |
3 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 21, 2024
As discussed in #32120 (comment), this adds the WPT `user-timing` tests to the default list of WPT tests, and also commits the current status of the `user-timing` tests. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
nolanlawson
force-pushed
the
nolan/clear-marks-squashed
branch
from
b1fd71f
to
e92fb1a
Compare
|
OK, now that #32123 is merged, I rebased on |
mrobinson
added
T-linux-wpt-2013
github-actions
bot
removed
T-linux-wpt-2013
|
🔨 Triggering try run (#8775225338) for Linux WPT |
|
Test results for linux-wpt-layout-2013 from try job (#8775225338): Flaky unexpected result (12)
Stable unexpected results that are known to be intermittent (15)
|
|
Test results for linux-wpt-layout-2020 from try job (#8775225338): Flaky unexpected result (17)
Stable unexpected results that are known to be intermittent (18)
|
|
✨ Try run (#8775225338) succeeded. |
mrobinson
approved these changes
Apr 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes several tests in wpt/user-timing by fixing some logic errors in how marks/measures are cleared (via
clearMarksandclearMeasures).There are two changes:
clear_entries_by_name_and_typeso that, whenclearMarks('foo')orclearMeasures('foo')is called, the presence of the entry name correctly filters based on existing entry names.entry_nameparam aDOMStringrather than anOption<DOMString>since every API call has it asSomeanyway, and I'm not aware of any Performance APIs where you can clear all entries regardless of type. (This is not strictly required for the fix, but I think it makes the code easier to read.)I also considered adding the expected WPT results using(Update: added!)mach update-wpt. But I'm not sure if you want these changes, since the expectations are currently missing (i.e.tests/wpt/meta/user-timingdoes not exist).For the record, this PR fixes the following tests:
clearMarks.html.iniclearMeasures.html.iniclear_non_existent_mark.any.js.iniclear_non_existent_measure.any.js.iniclear_one_mark.any.js.iniclear_one_measure.any.js.iniIn case you do want these meta files, here they are: nolanlawson@510e614./mach build -ddoes not report any errors./mach test-tidydoes not report any errors