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

Functional tests should be split to be more granular #670

Open
stof opened this issue Sep 28, 2023 · 3 comments
Open

Functional tests should be split to be more granular #670

stof opened this issue Sep 28, 2023 · 3 comments

Comments

@stof
Copy link
Member

stof commented Sep 28, 2023

Our functional tests are based on input files in https://github.com/scssphp/scssphp/tree/master/tests/inputs with a corresponding output CSS file in the outputs folder. Note that it does not support creating nested folders (the subfolders there are for partials that are imported by some tests).

Today, those tests are often not granular at all, making it hard to maintain the tests:

  • it is not clear what the test is testing
  • failures are all-or-nothing even when the test covers 20 different cases
  • often, the generated output does not even make it easy to identify which part of the file failed (in particular for builtins.scss which creates selectors with many times the same property so we don't know which one failed).
  • it makes it harder to compare the output to the reference implementation, for 2 reasons:
    • again, the all-or-nothing issue
    • for some cases, the failure is only related to formatting differences related to how different blocks are separated by empty lines or no. This is mostly solved for the compare-scss.sh file because it attempts at ignoring those empty lines. But this will affect us again when migrating to the new rendering in 2.0 which will change the formatting, leading to many file changes.

In addition to that, many things are already covered by equivalent (granular) tests in the official sass-spec testsuite.

Note that we also have some tests that are already properly granular (the more recent ones, as I stopped adding more things in the existing tests).

Work to do

extending_compound_selector.scss, color_operators.scss and comment_incomplete_interpolation.scss should not be reworked. They are covering deprecated behaviors of scssphp that are triggering errors in dart-sass (and so will also trigger errors in scssphp 2.0) so it is not worth spending time on them.

For each test that is not yet granular:

  1. identify what it attempts to test (for some of them, it might related to identifying the issue for which they act as a regression test). this will generally produce a list of things
  2. for each of those tested things:
    1. Check whether there is a test in the official sass testsuite.
    2. If yes, check whether we run that test or no in our testsuite (run phpunit -v tests/SassSpecTest.php --filter <name_of_the_test> and check whether the test is skipped or no. A quick check can also be done by checking in tests/specs/sass-spec-exclude.txt whether the test is excluded, but this exclusion list does not cover all skipped tests). If we run it, our duplicate functional test should be deleted as duplicate. If we skip it (often because of a formatting difference in the output), add a functional test using the input from the official testsuite, with a comment mentioning the name of the official test. See an example here
    3. If no, extract that in a dedicated scoped test, with a name describing what the test is covering. Please keep the file name short enough to satisfy filesystem requirements to avoid messing with contributors. If needed, the test file can start with a Sass silent comment (i.e. using //) providing a longer explanation
    4. If the new test is a regression test for a particular bug, it is good to provide a link to the issue in a silent comment at the beginning of the input
    5. Remove the old test code for that thing

A good check to do is to run the tests from tests/InputTest.php with code coverage enabled before and after the extraction of the granular tests, to ensure we don't regress on code coverage when splitting the test (when deduplicating based on sass-spec, the coverage check is harder to do include those specific tests too)

@stof stof changed the title Function tests should be split to be more granular Functional tests should be split to be more granular Sep 28, 2023
@stof
Copy link
Member Author

stof commented Sep 28, 2023

This work should happen in the 1.x branch, to make it easier to merge branches together and to see changes between 1.x and 2.0 when we will reach that point.

@stof
Copy link
Member Author

stof commented Sep 28, 2023

Note that I don't expect to see a big PR reworking everything at once here. Incremental progress is good.

@Cerdic
Copy link
Collaborator

Cerdic commented Sep 28, 2023

got it, I probably can help on this topic!

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

No branches or pull requests

2 participants