Skip to content
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

Add check that there are some resource entries #25547

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nhelfman
Copy link

Test resource_initiator_types doesn't check that any entries have returned from resrouce timing.
The test currently doesn't return stable number of entries but we need to at least ensure there are some entries.

Test resource_initiator_types doesn't check that any entries have returned from resrouce timing.
The test currently doesn't return stable number of entries but we need to at least ensure there are some entries.
Copy link
Contributor

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

Looks good modulo moving the assert into a test

@@ -135,6 +135,8 @@
addEntryIfExists(entries, expected_entries, pathname + 'empty.py?favicon', 'link');
addEntryIfExists(entries, expected_entries, pathname + 'eventsource.py?id=eventsource', 'other');

assert_greater_than(entries.length, 0, 'There should be resrouce entries.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert_greater_than(entries.length, 0, 'There should be resrouce entries.');
assert_greater_than(entries.length, 0, 'There should be resource entries.');

Noam-at-567404279722 added 3 commits September 22, 2020 17:19
Added performance observer to improve test reliability. Excluded eventSource type for simplicity
Since event source repeatedly triggers resource timing events we need to exclude it from the count and ensure at least one arrived.
@npm1
Copy link
Contributor

npm1 commented Sep 22, 2020

It's possible the Firefox failure is a flakiness in the implementation itself and not about the test being flaky, so in some sense it's possible that we may want to fix the lints and then force-merge. But some investigation is required in any case.

@plehegar plehegar removed their request for review March 25, 2022 19:33
@plehegar plehegar removed their assignment Mar 25, 2022
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.

None yet

4 participants