[test] Improve testing of Swift features #76740

drodriguez · 2024-09-27T00:15:45Z

Features in Swift can be experimental, upcoming or base features. During their lifetime features evolve from one category to other categories. Usually experimental features are only available in "asserts" compilers, but they can also be experimental features in "non asserts" compilers. Upcoming and base features are available in both "asserts" and "non asserts" compilers.

When coding a new feature, the feature normally will start as an experimental feature. In the past this has required to mark the tests that use that feature with REQUIRES: asserts to avoid the test failing when testing in a "non asserts" compilers. This requisite is not always follow, and we forget to add those REQUIRES: asserts from time to time. This causes breakages for people that test the "non asserts" compilers. For some experimental features that are available in production compilers the REQUIRES: asserts is not even needed, which can make adding those lines work against one intentions. When the feature graduates from experimental to upcoming or base, we sometimes forget to update the the related tests to remove those REQUIRES: asserts, so a "non asserts" compiler will not actually execute those tests, and bugs can be introduced by mistake in those compilers.

The changes in this PR introduce a system that aims to simplify testing those experimental features and avoid some of the problems noted above.

The first change is take the canonical Features.def and transform its contents in something that LLVM Lit can create available_features for. This is done abusing the Clang preprocessor to transform the .def file into a Python file that can be loaded by lit.site.cfg during testing. This is done for each build and will pick up changes in the Features.def as they happen, so it will always be up-to-date. Additionally it understand when features as available depending on "asserts" or "non asserts" compilers, and will not incorrectly require an "asserts" compiler for non-production features, or let experimental features be tested in a "non asserts" compiler.

The second part of the change is keeping the tests up-to-date with the features they are testing, so each test that uses a feature is marked as such. This is done with a test itself, which greps through the existing tests, checks for the usage of -enable-experimental-feature or -enable-upcoming-feature and warns the user about the missing REQUIRES: lines (failing the test suite, so nobody can submit a test that skips the requirements).

Finally, the last change is modifying a huge number of tests to follow the new rules. All the tests that currently use -enable-experimental-feature or -enable-upcoming-feature have been annotated with REQUIRES: lines, and the (now unnecessary) REQUIRES: asserts have been removed.

drodriguez · 2024-09-27T00:16:30Z

@swift-ci please test

drodriguez · 2024-09-27T02:43:12Z

@swift-ci please test

tshortli · 2024-09-27T05:12:38Z

This seems like a good idea to me.

test/ASTGen/attrs.swift

drodriguez · 2024-09-28T07:36:59Z

@swift-ci please test

drodriguez · 2024-09-28T16:26:49Z

@swift-ci please test

drodriguez · 2024-09-30T16:58:01Z

@swift-ci please test

drodriguez · 2024-09-30T22:31:13Z

@swift-ci please test

drodriguez · 2024-10-01T16:20:10Z

@swift-ci please test

drodriguez · 2024-10-03T22:58:08Z

@swift-ci please test

drodriguez · 2024-10-04T01:46:08Z

@swift-ci please test

tshortli · 2024-10-04T16:55:56Z

Would it be easy to separate out the REQUIRES: additions that are not strictly necessary from this PR? It seems like the bulk of the modified files in this PR are test cases that gained a REQUIRES: line where they previously did not have one. I'm not opposed in principle to consistently using the new REQUIRES: lines in every test that depends on a feature, but I think the additions to tests that didn't previously need any REQUIRES: lines make this PR somewhat harder to fully review. Separating out the mechanical changes into a follow up would help.

drodriguez · 2024-10-04T17:55:03Z

Would it be easy to separate out the REQUIRES: additions that are not strictly necessary from this PR? It seems like the bulk of the modified files in this PR are test cases that gained a REQUIRES: line where they previously did not have one. I'm not opposed in principle to consistently using the new REQUIRES: lines in every test that depends on a feature, but I think the additions to tests that didn't previously need any REQUIRES: lines make this PR somewhat harder to fully review. Separating out the mechanical changes into a follow up would help.

It is in part for simplicity (and as you point out consistency). The --enable-experimental-feature / --enable-upcoming-feature has some edge cases that are hard to deal with in an automatic way:

One can use --enable-experimental-feature for an upcoming feature because of compatibility (

swift/lib/Frontend/CompilerInvocation.cpp

Lines 1058 to 1064 in 54ef4dc

    
           // For compatibility, upcoming features can be enabled with the 
        
           // -enable-experimental-feature flag too since the feature may have 
        
           // graduated from being experimental. 
        
           if (auto feature = getUpcomingFeature(value)) { 
        
             if (enableUpcomingFeature(*feature)) 
        
               HadError = true; 
        
           }

). The REQUIRES: is not technically necessary.

Some --enable-experimental-feature can be used in production compilers.
--enable-upcoming-feature still works for features that are not longer upcoming (it prints a warning, but keeps working)

I use a script like the one in verify-swift-feature-testing.test-sh to apply the right REQUIRES: to each file (and to remove the REQUIRES: asserts), which does most of the legwork, but not all, and have to be manually re-adjusted for some cases. To account for those three edge cases, I would need to parse the contents of Features.def to figure out if the REQUIRES: is really necessary or not. This is not going to be easy in Bash (maybe it is not even possible), but I welcome ideas of how to approach that.

I can also split this PR in 2, but I tried to keep the three commits as "on topic" as possible to make review of the first and third commit as easy as possible. Since I have been working on this, I had to rebase and recalculate the second commit all the time, which I understand is difficult to review. I can maybe split it per feature "sets", if people think it is easier to review that way.

BTW, I was planning to probably merge this over a weekend or similar, since it is the moment less people are working on the repos and I can probably avoid anybody introducing another usage of an experimental feature in the tests while this PR is being tested.

drodriguez · 2024-10-04T17:57:44Z

@swift-ci please test

drodriguez · 2024-10-09T19:40:02Z

@swift-ci please test

drodriguez · 2024-10-11T20:49:18Z

@swift-ci please test

drodriguez · 2024-10-11T23:33:21Z

@swift-ci please test

drodriguez · 2024-11-01T22:06:54Z

Thinking in landing this over the weekend to be less disruptive, if nobody has more feedback and all the testing is green (I will probably rebase again and redo testing to avoid any race conditions landing with other code). I would appreciate a thumbs up, if nobody has any problems with the approach.

Take the `Features.def` file used in other parts of the code and create a file that can be used from the LLVM Lit configuration files to add new available features that can be checked from the tests with `REQUIRES` and others. The file `lit.swift-features.cfg.inc` is preprocessed by Clang and generates a file with Python syntax that can be loaded from both `lit.site.cfg.in` files. The preprocessing output is copied into the different test directories in the build directory, and added it is added as a dependency of them, so it will be generate when the test run or when `Features.def` changes. `EXPERIMENTAL_FEATURES` are only enabled if they are available in production or the compiler is being built with assertions, while `UPCOMING_FEATURES` and the rest of the `LANGUAGE_FEATURES` are always available.

Find all the usages of `--enable-experimental-feature` or `--enable-upcoming-feature` in the tests and replace some of the `REQUIRES: asserts` to use `REQUIRES: swift-feature-Foo` instead, which should correctly apply to depending on the asserts/noasserts mode of the toolchain for each feature. Remove some comments that talked about enabling asserts since they don't apply anymore (but I might had miss some). All this was done with an automated script, so some formatting weirdness might happen, but I hope I fixed most of those. There might be some tests that were `REQUIRES: asserts` that might run in `noasserts` toolchains now. This will normally be because their feature went from experimental to upcoming/base and the tests were not updated.

The test will look for other tests using `RUN:` lines that use experimental or upcoming features and will check that the tests also are checking with the right `REQUIRES:` lines for the used features. This should avoid new tests being introduced without the right `REQUIRES` and should avoid breaking the toolchain in less tested configurations.

Quickly explain the feature, its intended usage and a warning about not longer needing `REQUIRES: asserts` anymore.`

drodriguez · 2024-11-02T18:47:12Z

@swift-ci please test

tshortli · 2024-11-03T20:14:37Z

After pulling this down and rebuilding swift from scratch, I'm unable to run tests:

python3 ../llvm-project/llvm/utils/lit/lit.py --succinct --verbose ../build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64
lit.py: /Users/allan/Projects/swift/swift-project/llvm-project/llvm/utils/lit/lit/TestingConfig.py:152: fatal: unable to parse config file '/Users/allan/Projects/swift/swift-project/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/lit.site.cfg', traceback: Traceback (most recent call last):
  File "/Users/allan/Projects/swift/swift-project/llvm-project/llvm/utils/lit/lit/TestingConfig.py", line 140, in load_from_path
    exec(compile(data, path, "exec"), cfg_globals, None)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/allan/Projects/swift/swift-project/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/lit.site.cfg", line 186, in <module>
    lit_config.load_config(config, os.path.join(config.test_exec_root, "lit.swift-features.cfg"))
    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/allan/Projects/swift/swift-project/llvm-project/llvm/utils/lit/lit/LitConfig.py", line 154, in load_config
    config.load_from_path(path, self)
    ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
  File "/Users/allan/Projects/swift/swift-project/llvm-project/llvm/utils/lit/lit/TestingConfig.py", line 127, in load_from_path
    f = open(path)
FileNotFoundError: [Errno 2] No such file or directory: '/Users/allan/Projects/swift/swift-project/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/lit.swift-features.cfg'

drodriguez · 2024-11-04T16:56:17Z

FileNotFoundError: [Errno 2] No such file or directory: '/Users/allan/Projects/swift/swift-project/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/lit.swift-features.cfg'

Did you try with build-script --reconfigure maybe? https://github.com/swiftlang/swift/pull/76740/files#diff-33394812ba204689144fd2f80832db83853ba1cb32403edb4e15fe4893e675fdR321 is part of the CMake configuration phase. I would expect going through one ninja or cmake invocation would have recreated that file, but I am not sure if you executed one of those or directly lit.py.

tshortli · 2024-11-04T17:01:36Z

Yes, I made sure I re-ran with --reconfigure. The only way I was able to work around this was to invoke build-script with --test one time, which caused the necessary file to be produced.

drodriguez · 2024-11-04T17:08:46Z

Yes, I made sure I re-ran with --reconfigure. The only way I was able to work around this was to invoke build-script with --test one time, which caused the necessary file to be produced.

In

swift/CMakeLists.txt

Lines 1553 to 1554 in c0a55e1

    
           if(SWIFT_INCLUDE_TESTS) 
        
             add_subdirectory(test)

if SWIFT_INCLUDE_TESTS is not set, the full test/ subdirectory is skipped from CMake. I normally keep my presets with --test and --validation-test enabled, and then --skip-test-swift (and a bunch of others) so the tests do not actually run while building.

tshortli · 2024-11-04T17:23:01Z

I wonder if there is much of a benefit to skipping the test subdirectory in the CMake when --test is not specified? I think it's a bit awkward if everyone has to build with both --test and --skip-test-swift simultaneously specified in order to be able to run the tests locally. Not the end of the world, but I expect a lot of other contributors will hit the same confusion I did since it wasn't necessary previously.

drodriguez · 2024-11-04T17:27:11Z

I am surprised that it was working for you without going into that directory at least once before. The code for lit.swift-features.cfg is written to parallel the one for lit.site.cfg (https://github.com/swiftlang/swift/blob/main/test/CMakeLists.txt#L303) which would had also be not generated without --test. That said, I agree that configuring the tests and executing the tests should not be tied together in the build-script.

rintaro · 2024-11-05T00:26:23Z

KeyPathWithStaticMembers was removed from Features.def in 5d77f3c, so some tests became UNSUPPORTED unexpectedly.
@drodriguez , could you add a check to verify all the swift_feature_{Name} in each test are listed in Feature.def?

drodriguez · 2024-11-05T00:44:22Z

KeyPathWithStaticMembers was removed from Features.def in 5d77f3c, so some tests became UNSUPPORTED unexpectedly.

I was not expecting features disappearing from that file. I can try to see what I can do.

While doing swiftlang#76740 I iteratively was adding new `REQUIRES:` as new usages of the features were found, but I did not realize that at the same time other people might be removing some of those usages. The tests in this commit had some `REQUIRES:` line for a previous `-enable-experimental/upcoming-feature`, but they not longer use those features, so the `REQUIRES:` were effectively disabling the tests (at least in the case of `KeyPathWithStaticMembers`. In other cases they might still had executed).

Sometimes features are removed from `Features.def`, but they are not removed from the test files. The compiler ignores every feature that does not exist. This leads to removed features still being tested, and with the introduction of swiftlang#76740, `REQUIRED:`. Modify the code that generates the active feature set for testing to also generate the set of existing features, and pass this list of existing features to the verifier script. If a feature is trying to be activated and does not exist, the verifier will warn the user. - `SwiftParser` feature was renamed `ParserASTGen`, but some tests were using both spellings. Remove the mentions of `SwiftParser` in the tests. - `ImportObjcForwardDeclarations` was spelled with upper case `C` in a couple of tests. Fix it so the matching is easier. - `TransferringArgsAndResults` was an experimental feature that was removed. - Ignore the usage of inexisting feature in `swift-export-as.swift` because it seems to be what the test is actually testing. - Ignore the test `availability_define.swift` because it tests the pseudo-feature `AvailabilityMacro=` which is not part of `Features.def`.

Sometimes features are removed from `Features.def`, but they are not removed from the test files. The compiler ignores every feature that does not exist. This leads to removed features still being tested, and with the introduction of #76740, `REQUIRED:`. Modify the code that generates the active feature set for testing to also generate the set of existing features, and pass this list of existing features to the verifier script. If a feature is trying to be activated and does not exist, the verifier will warn the user. - `SwiftParser` feature was renamed `ParserASTGen`, but some tests were using both spellings. Remove the mentions of `SwiftParser` in the tests. - `ImportObjcForwardDeclarations` was spelled with upper case `C` in a couple of tests. Fix it so the matching is easier. - `TransferringArgsAndResults` was an experimental feature that was removed. - Ignore the usage of inexisting feature in `swift-export-as.swift` because it seems to be what the test is actually testing. - Ignore the test `availability_define.swift` because it tests the pseudo-feature `AvailabilityMacro=` which is not part of `Features.def`.

jamieQ · 2024-11-15T13:28:23Z

@drodriguez – apologies for hijacking this PR commentary, but i just hit the same issue @tshortli reported above after updating my checkout to f88b29b yesterday and re-running the build-script command i had last used to configure my checkout for development (yes, it was missing the --test flag). IMO this seems like a bit of a regression to the initial contributor experience since the guide seemingly does not indicate that passing the --test flag is required. granted i did not check out a commit preceding this change so i'm not entirely sure if this is exactly responsible. at any rate, i filed an issue here: #77642

drodriguez · 2024-11-15T17:45:53Z

See my answer in the forums https://forums.swift.org/t/missing-lit-config-file-after-swift-compiler-build/76019/14.

Were you running lit.py directly? While faster, it will not rebuild the dependencies of the tests. Only CMake knows how to do that.

jamieQ · 2024-11-17T11:44:15Z

See my answer in the forums https://forums.swift.org/t/missing-lit-config-file-after-swift-compiler-build/76019/14.

Were you running lit.py directly? While faster, it will not rebuild the dependencies of the tests. Only CMake knows how to do that.

thanks for the feedback! as best i can tell from the shell history i have left, i ran it via utils/run-test --lit ... and then encountered the error.

drodriguez · 2024-11-18T17:25:42Z

I was trying to change things a little to avoid the failure in #77658. Hopefully that will avoid most of the errors, but it will hide whatever missing dependency is happening in your (and many others) case. I simply cannot repro the problem people are having.

drodriguez force-pushed the test-swift-features branch from e7c0602 to 14e2017 Compare September 27, 2024 02:43

tshortli reviewed Sep 27, 2024

View reviewed changes

test/ASTGen/attrs.swift Outdated Show resolved Hide resolved

drodriguez force-pushed the test-swift-features branch from 14e2017 to e6de4f2 Compare September 28, 2024 07:36

drodriguez force-pushed the test-swift-features branch from e6de4f2 to 8ca4461 Compare September 28, 2024 16:26

drodriguez force-pushed the test-swift-features branch from 8ca4461 to 647cfde Compare September 30, 2024 16:57

drodriguez force-pushed the test-swift-features branch from 647cfde to 8bfab1b Compare September 30, 2024 22:30

drodriguez force-pushed the test-swift-features branch from 8bfab1b to d4e1fa8 Compare October 1, 2024 02:14

drodriguez force-pushed the test-swift-features branch from d4e1fa8 to 360656c Compare October 3, 2024 22:57

drodriguez force-pushed the test-swift-features branch from 360656c to 5208bd6 Compare October 4, 2024 01:45

drodriguez force-pushed the test-swift-features branch from 5208bd6 to ac667a0 Compare October 4, 2024 17:56

drodriguez force-pushed the test-swift-features branch 2 times, most recently from d2126fb to 6478445 Compare October 9, 2024 19:39

drodriguez force-pushed the test-swift-features branch from 6478445 to 4deecc9 Compare October 11, 2024 20:49

drodriguez force-pushed the test-swift-features branch from 4deecc9 to 1304a4f Compare October 11, 2024 23:32

tshortli approved these changes Nov 1, 2024

View reviewed changes

drodriguez added 4 commits November 2, 2024 11:41

[docs] Add a couple of paragraphs about swift_feature in testing.md

2f62bf4

Quickly explain the feature, its intended usage and a warning about not longer needing `REQUIRES: asserts` anymore.`

drodriguez force-pushed the test-swift-features branch from 68acf40 to 2f62bf4 Compare November 2, 2024 18:47

drodriguez merged commit 48415c9 into swiftlang:main Nov 3, 2024
5 checks passed

eeckstein mentioned this pull request Nov 4, 2024

tests: add a missing REQUIRE in SILOptimizer/stack_promotion_isolated_deinit.swift #77366

Merged

drodriguez mentioned this pull request Nov 5, 2024

[test] Fail on test requiring features that are not used or had disappeared #77380

Merged

drodriguez deleted the test-swift-features branch November 6, 2024 01:15

drodriguez mentioned this pull request Nov 11, 2024

[test] Warning about Swift features that do not exist #77542

Merged

jamieQ mentioned this pull request Nov 15, 2024

Enable configuring tests without running them & improve GettingStarted guide's accuracy regarding tests #77642

Closed

[test] Improve testing of Swift features #76740

[test] Improve testing of Swift features #76740

Uh oh!

Conversation

drodriguez commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguez commented Sep 27, 2024

Uh oh!

drodriguez commented Sep 27, 2024

Uh oh!

tshortli commented Sep 27, 2024

Uh oh!

Uh oh!

drodriguez commented Sep 28, 2024

Uh oh!

drodriguez commented Sep 28, 2024

Uh oh!

drodriguez commented Sep 30, 2024

Uh oh!

drodriguez commented Sep 30, 2024

Uh oh!

drodriguez commented Oct 1, 2024

Uh oh!

drodriguez commented Oct 3, 2024

Uh oh!

drodriguez commented Oct 4, 2024

Uh oh!

tshortli commented Oct 4, 2024

Uh oh!

drodriguez commented Oct 4, 2024

Uh oh!

drodriguez commented Oct 4, 2024

Uh oh!

drodriguez commented Oct 9, 2024

Uh oh!

drodriguez commented Oct 11, 2024

Uh oh!

drodriguez commented Oct 11, 2024

Uh oh!

drodriguez commented Nov 1, 2024

Uh oh!

drodriguez commented Nov 2, 2024

Uh oh!

Uh oh!

tshortli commented Nov 3, 2024

Uh oh!

drodriguez commented Nov 4, 2024

Uh oh!

tshortli commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drodriguez commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tshortli commented Nov 4, 2024

Uh oh!

drodriguez commented Nov 4, 2024

Uh oh!

rintaro commented Nov 5, 2024

Uh oh!

drodriguez commented Nov 5, 2024

Uh oh!

jamieQ commented Nov 15, 2024

Uh oh!

drodriguez commented Nov 15, 2024

Uh oh!

jamieQ commented Nov 17, 2024

Uh oh!

drodriguez commented Nov 18, 2024

Uh oh!

Uh oh!

drodriguez commented Sep 27, 2024 •

edited

Loading

tshortli commented Nov 4, 2024 •

edited

Loading

drodriguez commented Nov 4, 2024 •

edited

Loading