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

Split all tests into separate files #227

Merged
merged 1 commit into from
Jan 27, 2022
Merged

Split all tests into separate files #227

merged 1 commit into from
Jan 27, 2022

Conversation

SebastianZ
Copy link
Collaborator

While splitting them they were also converted into ES modules.

This also came with some consequences. The Array prototype extensions times() and and() got removed and the tests that used them now include the complete arrays previously produced by those methods.
Furthermore, the ele() function got replaced by Bliss' $() function.

I probably could have moved them into some kind of helper module, though I believe those functions aren't necessary.

I only turned the tests into modules in this change, as I focused on #219. Though other parts of the code should probably also be converted into modules (as part of #216).

This change will cause conflicts with the other PRs, so depending on which ones are merged first, some rebasing needs to happen.

This closes #219.

Sebastian

@LeaVerou
Copy link
Owner

Beautiful.

@Zefling
Copy link
Collaborator

Zefling commented Jan 23, 2022

It's a shame that with this mode, it's no longer stand alone (file://) and that we need a server to test.

@SebastianZ
Copy link
Collaborator Author

SebastianZ commented Jan 24, 2022

It's a shame that with this mode, it's no longer stand alone (file://) and that we need a server to test.

Right, though requiring a server shouldn't be a blocker for this. If you don't have or want to run a full-fledged server, there are also various ways to start a server from within your IDE.
E.g. in VSCode there's a plugin called Live Server, which allows you to start a server for the currently open folder with a single click.

@Zefling Is that your only concern or do you have other feedback?

Sebastian

@Zefling
Copy link
Collaborator

Zefling commented Jan 24, 2022

It's just an observation. Maybe write a note in the readme.

No problem other than that.

Copy link
Collaborator

@PolariTOON PolariTOON left a comment

Choose a reason for hiding this comment

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

I understand re-exporting all the specs from one place (tests.js) is convenient for legibility but i think this is far from being ideal from a performance point of view, because this means these 105 (!) imports are actually discovered by the browser after two consecutive subrequests: csstest.js, and then tests.js.

The easier way i see to improve that is to import the specs directly from csstest.js.
Or we could preload some (all?) the files with <link rel="modulepreload">, but the support for it is not great currently.

Side note: do we want to use spaces for indentation and CRLF line endings in this project now? 😅 I don't really care, but i would just prefer consistency across the project.

@LeaVerou
Copy link
Owner

Importing the same module twice does not fire a separate HTTP request. Also, isn't the whole point of HTTP/2 that multiple HTTP requests are fine? 😄

Side note: do we want to use spaces for indentation and CRLF line endings in this project now?

Oof, let's not please. I must have missed that when reviewing the PR.

@SebastianZ
Copy link
Collaborator Author

I understand re-exporting all the specs from one place (tests.js) is convenient for legibility but i think this is far from being ideal from a performance point of view, because this means these 105 (!) imports are actually discovered by the browser after two consecutive subrequests: csstest.js, and then tests.js.

The easier way i see to improve that is to import the specs directly from csstest.js.

tests.js is just one additional request, so it doesn't make much of a change to remove it. I could remove it, though I kept it for readablility and to keep the Specs structure unchanged.

Or we could preload some (all?) the files with <link rel="modulepreload">, but the support for it is not great currently.

I agree we should add <link rel="modulepreload">s. In browsers that don't support it it's just ignored, so there's no harm in adding it. I'd add one for all tests except the CSS 2 ones because they are excluded by default.

Side note: do we want to use spaces for indentation and CRLF line endings in this project now? 😅 I don't really care, but i would just prefer consistency across the project.

That's a mistake. My IDE wasn't set up correctly. I'll change the indentations back.

Sebastian

@SebastianZ SebastianZ force-pushed the split-tests branch 4 times, most recently from 258f62a to a1a3f79 Compare January 25, 2022 21:20
@SebastianZ
Copy link
Collaborator Author

Or we could preload some (all?) the files with <link rel="modulepreload">, but the support for it is not great currently.

I agree we should add <link rel="modulepreload">s. In browsers that don't support it it's just ignored, so there's no harm in adding it. I'd add one for all tests except the CSS 2 ones because they are excluded by default.

I've tested that now in Chrome 97 and Firefox 96 and didn't see any performance gain. Therefore I do not add them. @PolariTOON If you think we can somehow get a performance improvement by preloading resources, I suggest you open a separate issue and we'll discuss it there.

Side note: do we want to use spaces for indentation and CRLF line endings in this project now? 😅 I don't really care, but i would just prefer consistency across the project.

That's a mistake. My IDE wasn't set up correctly. I'll change the indentations back.

The indentation is fixed now.

Sebastian

Copy link
Collaborator

@PolariTOON PolariTOON left a comment

Choose a reason for hiding this comment

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

The total number of features tested appears to have decreased, because the following specs are not exported by tests.js:

[
  "Box Sizing Level 3",
  "Box Sizing Level 4",
  "Custom Properties for Cascading Variables Level 1",
  "Scoping Level 1",
  "Scroll Anchoring Level 1",
  "Scroll Snap Level 1",
  "Scrollbars Level 1",
  "Shadow Parts",
  "Shapes Level 1",
  "Shapes Level 2",
  "Will Change Level 1",
  "Writing Modes Level 3",
  "Writing Modes Level 4"
]

If you think we can somehow get a performance improvement by preloading resources, I suggest you open a separate issue and we'll discuss it there.

Sure, I will open an issue about it; i don't want to block this PR because of that.

@PolariTOON
Copy link
Collaborator

Also, I forgot to tell it above, but filter.js and files under tests/ still use Windows line endings.

@SebastianZ
Copy link
Collaborator Author

Also, I forgot to tell it above, but filter.js and files under tests/ still use Windows line endings.

Thank you for the hint! Line breaks should be Linux line endings all over the place now.

Sebastian

While splitting them they were also converted into ES modules.
@SebastianZ
Copy link
Collaborator Author

The total number of features tested appears to have decreased, because the following specs are not exported by tests.js:

[
  "Box Sizing Level 3",
  "Box Sizing Level 4",
  "Custom Properties for Cascading Variables Level 1",
  "Scoping Level 1",
  "Scroll Anchoring Level 1",
  "Scroll Snap Level 1",
  "Scrollbars Level 1",
  "Shadow Parts",
  "Shapes Level 1",
  "Shapes Level 2",
  "Will Change Level 1",
  "Writing Modes Level 3",
  "Writing Modes Level 4"
]

Total fail by me. 😞 Thank you so much for checking! I've included them now.

Sebastian

@SebastianZ SebastianZ merged commit 5293ae4 into gh-pages Jan 27, 2022
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.

Split tests into several files
4 participants