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

Improved testing options #460

Merged
merged 42 commits into from Apr 5, 2024
Merged

Improved testing options #460

merged 42 commits into from Apr 5, 2024

Conversation

tunetheweb
Copy link
Member

@tunetheweb tunetheweb commented Apr 4, 2024

Improved version of #286

Changes:

  • Allows you to run: npm run test:e2e -- --browser=chrome --metric=TTFB for example to limit tests to a particular browser and/or metric.
  • Adds a precommit test for any stray it.only's, depends.only's, or browser.debug's accidentally left around while testing.
  • Adds instructions to contributing file.
  • Moves Code of Conduct and Contributing files to more commonly known locations to make them easier to find.
  • Adds a unit test github action (and also updates github actions versions while at it).

Copy link

@nucliweb nucliweb left a comment

Choose a reason for hiding this comment

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

👏

@philipwalton
Copy link
Member

@tunetheweb I made a few tweaks to the code that I thought make things a bit clearer. Let me know if you have any issues with these changes, otherwise feel free to merge.

@tunetheweb
Copy link
Member Author

@philipwalton , I'll see your one array and raise you some more :-)

@tunetheweb
Copy link
Member Author

tunetheweb commented Apr 5, 2024

I also added a unit test GitHub action.

I tried the e2e test but couldn't get it working. Even restricting it to Chrome required a number of extra options ( args: ['headless', 'disable-gpu', 'remote-debugging-pipe'],) and still fails CLS tests (though the others pass). Let's leave for now and might revisit later. Apparently it can be a bit flakey anyway, plus our tests are a little flakey themselves, so maybe not a good fit for CI.

@tunetheweb
Copy link
Member Author

@philipwalton happy to merge? Or wanna have one last look after latest changes?

@tunetheweb
Copy link
Member Author

Actually I got the E2E tests working too. With one question.

test/e2e/onCLS-test.js Outdated Show resolved Hide resolved
test/e2e/onCLS-test.js Outdated Show resolved Hide resolved
@@ -662,11 +662,14 @@ describe('onCLS()', async function () {
if (!browserSupportsCLS) this.skip();

await navigateTo('/test/cls?hidden=1');
// Wait for a frame to be painted.
await nextFrame();
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why the nextFrame() is needed here? Given that the page is loaded in the hidden state, I wonder if waiting for the load event would be better?

You could try navigateTo('/test/cls?hidden=1', {readyState: 'complete'}) and see if that helps.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done!

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually let me see if that fixes the need for the >= mentioned above...

@tunetheweb tunetheweb merged commit 5420b5f into v4 Apr 5, 2024
6 checks passed
@tunetheweb tunetheweb deleted the improved-test-options branch April 5, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants