Implement some way to run UI tests ignoring run-pass tests #54047

Activity

1. petrochenkov

   added

   A-testsuiteArea: The testsuite used to check the correctness of rustcArea: The testsuite used to check the correctness of rustc

   on Sep 8, 2018
2. petrochenkov commented on Sep 8, 2018

   

   petrochenkov

   on Sep 8, 2018

   Last edited by petrochenkov

   ContributorAuthor

   More actions

   cc @pnkfelix @estebank
3. pnkfelix

   added

   T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.Relevant to the compiler team, which will review and decide on the PR/issue.

   on Sep 12, 2018
4. pnkfelix commented on Sep 12, 2018

   

   pnkfelix

   on Sep 12, 2018

   Last edited by pnkfelix

   Contributor

   More actions

   • (1a) One option: make a separate ui-run-pass suite and move this stuff there. That is perhaps the simplest to implement, but I don't know if it has the ideal developer UX.
     • (1b) A variant of this: reorganize ui/ to have subdirectories ui/compile-fail/, ui/compile-pass/, ui/run-pass/, ... and then move everything there. Then a developer should be able to opt into the subarea they are interested in, like compile-fail, by specifying it on their command line.
   • (2a) Another (orthogonal) option: add a flag that makes compiletest treat // run-pass as if it said // compile-pass. This would resolve the time overhead from running the tests.
     • (2b) A variant of this would provide flags to let the user say things like "only run the // compile-fail tests" or "skip the // run-pass tests...

   Of the above choices, I think I like (1b) the best...

   ---

   Anyway, my apologies: My whole strategy of resolving #53764 by migrating the run-pass tests to a subdirectory of ui/ was based on the presumption that the ui tests don't represent a huge part of our autobuild infrastructure overhead. While I did ask the infrastructure team whether this presumption was correct, the whole problem is that that presumption was not sufficient; it considered solely the overhead from our autobuild infrastructure, and failed to consider overall developer UX.
5. pnkfelix commented on Sep 12, 2018

   

   pnkfelix

   on Sep 12, 2018

   Contributor

   More actions

   Nominating for discussion at T-compiler meeting.
6. pnkfelix

   added

   I-nominated

   on Sep 12, 2018
7. pnkfelix commented on Sep 12, 2018

   

   pnkfelix

   on Sep 12, 2018

   Contributor

   More actions

   (actually, after further reflection, I'm starting to warm up to option (1a). After all, we started with a model where we had separate compile-fail and run-pass suites. So it would make sense to retain that division, and just think of this as "upgrading" each of the suites to test the compiler UX as well...)
8. petrochenkov commented on Sep 12, 2018

   

   petrochenkov

   on Sep 12, 2018

   Last edited by petrochenkov

   ContributorAuthor

   More actions

   I remember that @nikomatsakis wanted to avoid variants like 1a/b to introduce a per-feature directory structure instead:

   /ui
       /rfc-XXXX-feature-name1
           *both compile-fail and run-pass tests for this feature*
       /rfc-YYYY-feature-name1
           *both compile-fail and run-pass tests for this feature*
       /rfc-ZZZZ-feature-name1
           *both compile-fail and run-pass tests for this feature*
   

   with run-pass/compile-pass/compile-fail discerned by in-file annotations (i.e. like it works right now basically).
   With this scheme compiletest/rustbuild would need to regain the ability to run test groups separately, but now using those annotations.
9. pnkfelix commented on Sep 12, 2018

   

   pnkfelix

   on Sep 12, 2018

   Last edited by pnkfelix

   Contributor

   More actions

   I do notice that compiletest has a --mode argument which is implemented as only accepting one of "(compile-fail|parse-fail|run-fail|run-pass|run-pass-valgrind|pretty|debug-info|incremental|mir-opt)",

   I suspect that is implemented by inspecting directory names, but since it does not list ui, we could expand its meaning to also mean "inspect the header of the ui/ tests to determine its catgory." That might give us a straight-forward way to implement option (2b) that I recently added to my comment above.

   Update: Oops, the documentation doesn't mention ui, but the implementation does have a Mode::Ui variant, and the impl FromStr for Mode will indeed translate "ui" to Mode::Ui.

   Still, I am warming up to this new approach to implementing option (2b).
10. nikomatsakis commented on Sep 13, 2018

    

    nikomatsakis

    on Sep 13, 2018

    Contributor

    More actions

    My feeling here:

    • I dislike dividing up our test suite into directories like run-pass, compile-fail, etc
    • I would rather have the "primary organization" of the tests be the area of code that they are testing (e.g., borrowck, regionck, some RFC, etc)

    This leads me to suggest that we ought to add a "test filter" that lets you pare down the tests to exclude run-pass or other modes. In fact, if we included the "mode" in the name of the test, that would happen automatically via --test-args, but that might break other things.

    e.g., the name of a // run-pass test called foo/bar.rs could be foo/bar.rs (run-pass) or something like that. Then you could do --test-args '(run-pass)'.

    (Of course, there really is no ideal way to organize the tests. But organizing by "what the tests are testing" has advantages when you're trying to look over the set of tests for a given feature, and using --test-args to filter seems to fulfill the other use cases. I can't think of a time when I'm like "I'd like to browse the set of tests that don't generate errors".)
11. pnkfelix

    added

    P-highHigh priorityHigh priority

    and removed

    I-nominated

    on Sep 13, 2018
12. pnkfelix

    self-assigned this

    on Sep 13, 2018
13. pnkfelix commented on Sep 13, 2018

    

    pnkfelix

    on Sep 13, 2018

    Contributor

    More actions

    discussed at meeting. P-high. I will work on addressing @petrochenkov 's developer UX concern in short term.
14. pnkfelix commented on Sep 13, 2018

    

    pnkfelix

    on Sep 13, 2018

    Last edited by pnkfelix

    Contributor

    More actions

    namely, my short term plan is:

    1. Turn src/test/run-pass/ into "another ui-suite. Any test that cannot deal with such a transition should be moved elsewhere.
       • This is PR uiify run-pass #54223. (I ended up adding workarounds so that all tests could stay in that directory.)
    2. Move everything I moved to src/test/ui/run-pass/ back to the new ui-ified src/test/run-pass/.

    That the short-term plan.

    The long term plan will involve engaging the broader compile team to figure out how we want to organize all of our tests.
15. 3 remaining items

    Load more
16. pnkfelix commented on Sep 20, 2018

    

    pnkfelix

    on Sep 20, 2018

    Contributor

    More actions

    @RalfJung yes, one of my assumptions is that as part of a broader reworking, we'll set things up so that one can opt into skipping e.g. all tests that have // run-pass in their header (or as one of their properties; however that knowledge is encoded...)
17. pnkfelix commented on Sep 20, 2018

    

    pnkfelix

    on Sep 20, 2018

    Contributor

    More actions

    visited for triage. The plan is still to move src/test/ui/run-pass/ back to src/test/run-pass/ once PR #54223 lands.

    However, given that landing PR #54223 hasn't been a seamless process: If #54223 hasn't landed in a week's time, then I will immediately move src/test/ui/run-pass/ back to src/test/run-pass at that point, in order to stop inconveniencing the developer UX for people focused on compile-fail issues in the ui suite.

    👍React with 👍1Reacted by Esteban Kuber
18. pnkfelix commented on Sep 24, 2018

    

    pnkfelix

    on Sep 24, 2018

    Contributor

    More actions

    yay #54223 landed. Going to move src/test/ui/run-pass/* back to src/test/run-pass/* now.
19. pnkfelix commented on Sep 27, 2018

    

    pnkfelix

    on Sep 27, 2018

    Contributor

    More actions

    visited for triage. making progress; short term plan should be finished once PR #54530 lands.
20. bors

    closed this as completedin #54530on Sep 28, 2018