./x.py test should be able to run individual tests #49729

collin5 · 2018-04-06T14:09:52Z

Allows user to be able to run individual tests by specifying filename i.e ./x.py test src/test/run-pass/foo.rs

rust-highfive · 2018-04-06T14:10:03Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

alexcrichton · 2018-04-06T18:56:51Z

Thanks for the PR! I've long wanted to do this!

I think though this may be best done in rustbuild itself because we can't always just chop off one filename, but instead may have to chop off a few to get to the name of the test suite. (which the python script doens't know about but rustbuild does)

collin5 · 2018-04-06T21:11:38Z

Going to try doing this. Thank you 👍

collin5 · 2018-04-13T11:22:32Z

Hi @alexcrichton, I've made the requested changes. PR should now be ready for another review. Thank you.

alexcrichton · 2018-04-13T16:16:11Z

Thanks! Looks reasonable to me, but to double check...

r? @Mark-Simulacrum

Mark-Simulacrum · 2018-04-13T17:16:46Z

src/bootstrap/test.rs

+        // Get test-args by striping suite path
+        let mut test_args: Vec<&str> = vec![];
+        paths.iter().filter(|p| p.starts_with(path) && p.is_file())
+          .for_each(|p| test_args.push(&p.strip_prefix(path).unwrap().to_str().unwrap()));


This should be possible the rewrite as a collect.

Mark-Simulacrum · 2018-04-13T17:19:21Z

src/bootstrap/builder.rs

@@ -70,6 +70,8 @@ pub trait Step: 'static + Clone + Debug + PartialEq + Eq + Hash {
    /// Run this rule for all hosts without cross compiling.
    const ONLY_HOSTS: bool = false;

+    const PATH: &'static str = "not set";


I would rather not add this here as it feels like the wrong place to put it, could you try instead having it returned as part of ShouldRun? I think that should be relatively minimally impactful on the code, just moving a few things around. Calling should_run multiple times is fine. I'd also appreciate renaming it to suite_path and making it an Option.

Please also verify that a mix of specific tests and crates works: ./x.py test src/test/run-pass/foo.rs src/bootstrap should test both the specific test and bootstrap. I think the current code might not quite do this, but I'm not sure.

Thanks! I'm on it

rust-highfive · 2018-04-15T09:08:39Z

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

Resolving deltas: 100% (615027/615027), completed with 4886 local objects.
---
[00:00:51] configure: rust.quiet-tests     := True
---
[00:05:49] tidy error: /checkout/src/bootstrap/test.rs:927: line longer than 100 chars
[00:05:51] some tidy checks failed
[00:05:51]
[00:05:51]
[00:05:51] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:51] expected success, got: exit code: 1
[00:05:51]
[00:05:51]
[00:05:51] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:51] Build completed unsuccessfully in 0:02:27
[00:05:51] Makefile:79: recipe for target 'tidy' failed
[00:05:51] make: *** [tidy] Error 1
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:0bfa28ef:start=1523783239475498643,finish=1523783239482607040,duration=7108397
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:0a59e890
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:0a59e890:start=1523783239488911685,finish=1523783239496142286,duration=7230601
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0339a260
$ dmesg | grep -i kill
[   10.599429] init: failsafe main process (1094) killed by TERM signal

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

collin5 · 2018-04-15T14:06:56Z

@Mark-Simulacrum Thanks! I've worked on the feedback. Also mix of specific tests and crates i.e /x.py test src/test/run-pass/foo.rs src/bootstrap should now work just fine 👍

Mark-Simulacrum

I think a few more comments, but this is looking better! Thanks for working through this.

Mark-Simulacrum · 2018-04-15T15:08:21Z

src/bootstrap/builder.rs

+                for p in paths {
+                    if p.starts_with(&suite) {
+                        suites.push(PathBuf::from(suite));
+                        break


This feels wrong to me; if ./x.py test src/test/run-pass src/test/compile-fail is invoked I'd expect the break to exit too early for us to populate suites for both tests.

It'd would also somewhat surprise me if this will work as-is when given a path to say src/libstd since those are all set as individual paths inside the pathset, whereas the "suite" detection only matches the last path given. It might be best to restructure to have suite-matching be separate and orthogonal to the path-matching to avoid that problem.

Hmm, Could you please clarify on this if you don't mind.

Basically, the way this code handles suites conflates them with the paths that could invoke a given Step. I think that's the wrong distinction to have as it adds little benefit and increases maintenance burden by connecting two concepts that are mostly unrelated. Instead, I would rather see these modifications contained to changes in maybe_run and the code that calls it. Essentially, I'd expect another case added that calls should_run.is_suite_path(path) or something like and then maybe_run can be called with a PathSet::Suite (making PathSet an enum). Does that help clarify? I can maybe explain in further detail.

Sorry for the somewhat moving target here!

It's alright. I now get it. Thanks! Will let you know if I get any more questions.

Mark-Simulacrum · 2018-04-15T15:09:57Z

src/bootstrap/test.rs

+
+        // Get test-args by striping suite path
+        let mut test_args: Vec<&str> = paths.iter().filter(|p| p.starts_with(suite_path) &&
+           p.is_file()).map(|p| p.strip_prefix(suite_path).unwrap().to_str().unwrap()).collect();


We should probably error out here if the argument is going to be silently ignored because it's part of our suite yet not a file (and as such we don't know what to do with it).

Mark-Simulacrum · 2018-04-15T15:10:27Z

src/bootstrap/test.rs

+        let mut test_args: Vec<&str> = paths.iter().filter(|p| p.starts_with(suite_path) &&
+           p.is_file()).map(|p| p.strip_prefix(suite_path).unwrap().to_str().unwrap()).collect();
+
+        if test_args.is_empty() {


Any additional test args passed the command line should rather be appended to the existing set of paths.

Thanks! going to fix this ASAP

bors · 2018-04-18T08:31:47Z

☔ The latest upstream changes (presumably #49972) made this pull request unmergeable. Please resolve the merge conflicts.

collin5 · 2018-04-24T21:05:37Z

Hi @Mark-Simulacrum, sorry for the delay on this, I've been kind of busy lately. You can now take another look. Thanks

Mark-Simulacrum · 2018-04-26T20:34:20Z

src/bootstrap/builder.rs

-        self.set.iter().any(|p| p.ends_with(needle))
+        match self {
+            PathSet::Set(set) => set.iter().any(|p| p.ends_with(needle)),
+            _ => false


Please be explicit about which variant's are false here.

Mark-Simulacrum · 2018-04-26T20:34:40Z

src/bootstrap/builder.rs

-        self.set.iter().next().unwrap_or(&builder.build.src).to_path_buf()
+        match self {
+            PathSet::Set(set) => set.iter().next().unwrap_or(&builder.build.src).to_path_buf(),
+            _ => PathBuf::from("")


Let's instead return an Option from path or panic if we don't expect it to be called on suites.

Mark-Simulacrum · 2018-04-26T20:35:48Z

src/bootstrap/builder.rs

+                    if let Some(suite) = should_run.is_suite_path(path) {
+                        attempted_run = true;
+                        desc.maybe_run(builder, suite);
+                    }
                    if let Some(pathset) = should_run.pathset_for_path(path) {


Just for sanity's sake, let's make this an else perhaps? I think it'll make it easier to reason about when steps are run.

Mark-Simulacrum · 2018-04-26T20:36:44Z

src/bootstrap/test.rs

@@ -672,12 +674,13 @@ macro_rules! test_definitions {
                });
            }

-            fn run(self, builder: &Builder) {
+            fn run(self,builder: &Builder) {


nit: space required

Mark-Simulacrum · 2018-04-26T20:38:44Z

src/bootstrap/test.rs

+
+        // Get test-args by striping suite path
+        let is_file = |p: &PathBuf| {
+            assert!(p.is_file(), "{:?} is not a valid file", p);


Let's change the message here to be Expected {:?} to be a path to a test file to be clearer about the reason for this error.

Mark-Simulacrum · 2018-04-26T20:39:19Z

src/bootstrap/test.rs

+        };
+
+        // Get test-args by striping suite path
+        let is_file = |p: &PathBuf| {


Please change this to assert_file to be clearer.

Mark-Simulacrum · 2018-04-26T20:39:31Z

src/bootstrap/builder.rs

@@ -7,7 +7,6 @@
 // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
-


nit: This line should stay.

collin5 · 2018-04-27T11:52:06Z

Thanks! Fixed.

shepmaster · 2018-05-06T22:27:58Z

Ping from triage, @Mark-Simulacrum — looks like it's ready again!

Mark-Simulacrum · 2018-05-07T00:26:28Z

Thank you for taking this on! Let me know if you'd like another project in rustbuild to work on (there's a few cleanups that should be relatively straightforward, if you're interested); we can also put you in touch with other rust teams/working groups if you'd like to try something different.

@bors r+

bors · 2018-05-07T00:26:29Z

📌 Commit 281f534 has been approved by Mark-Simulacrum

rust-highfive · 2018-05-07T21:22:09Z

The job asmjs of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:04:01] DirectMap2M:     3074048 kB
[00:04:01] DirectMap1G:    14680064 kB
[00:04:02] + python2.7 ../x.py test --target asmjs-unknown-emscripten src/test/run-pass src/test/run-fail src/libstd src/liballoc src/libcore
[00:04:02]     Finished dev [unoptimized] target(s) in 0.0 secs
[00:04:03] thread 'main' panicked at 'Expected "src/test/run-pass" to be a path to a test file', bootstrap/test.rs:1015:13
[00:04:03] travis_fold:end:log-system-info
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target asmjs-unknown-emscripten src/test/run-pass src/test/run-fail src/libstd src/liballoc src/libcore
[00:04:03] Build completed unsuccessfully in 0:00:01
travis_time:end:0f7434aa:start=1525727844324493913,finish=1525728087973454601,duration=243648960688

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

collin5 · 2018-05-07T22:17:59Z

src/bootstrap/test.rs

+
+        // Get test-args by striping suite path
+        let assert_file = |p: &PathBuf| {
+            assert!(p.is_file(), "Expected {:?} to be a path to a test file", p);


Hi @Mark-Simulacrum Sorry, I missed this. Looks like this panics with ./x.py test suite_path I think we should just ignore test args if suite path is specified.

rust-highfive · 2018-05-08T12:41:31Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:05:37] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:38] tidy error: /checkout/src/bootstrap/test.rs:1015: line longer than 100 chars
[00:05:39] some tidy checks failed
[00:05:39] 
[00:05:39] 
[00:05:39] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:39] 
[00:05:39] 
[00:05:39] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:39] Build completed unsuccessfully in 0:02:28
[00:05:39] Build completed unsuccessfully in 0:02:28
[00:05:39] make: *** [tidy] Error 1
[00:05:39] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:08b2d318
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Mark-Simulacrum · 2018-05-08T15:00:51Z

@bors r+

bors · 2018-05-08T15:00:52Z

📌 Commit bd8f2a0 has been approved by Mark-Simulacrum

collin5 · 2018-05-09T20:43:16Z

Fixed! r? @Mark-Simulacrum
cc @kennytm

Mark-Simulacrum · 2018-05-09T21:01:58Z

@bors r+

bors · 2018-05-09T21:01:59Z

📌 Commit 2f8c2a9 has been approved by Mark-Simulacrum

bors · 2018-05-10T02:15:47Z

⌛ Testing commit 2f8c2a9 with merge 95d0b9e...

./x.py test should be able to run individual tests Allows user to be able to run individual tests by specifying filename i.e `./x.py test src/test/run-pass/foo.rs` Fixes #48483

bors · 2018-05-10T04:42:54Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing 95d0b9e to master...

rust-highfive assigned alexcrichton Apr 6, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2018

collin5 force-pushed the b48483 branch 2 times, most recently from f565e32 to db57d81 Compare April 13, 2018 09:04

rust-highfive assigned Mark-Simulacrum and unassigned alexcrichton Apr 13, 2018

Mark-Simulacrum reviewed Apr 13, 2018

View reviewed changes

collin5 force-pushed the b48483 branch 2 times, most recently from ff88d09 to 312e32f Compare April 15, 2018 08:59

collin5 force-pushed the b48483 branch from 312e32f to aa8f3d0 Compare April 15, 2018 09:12

Mark-Simulacrum reviewed Apr 15, 2018

View reviewed changes

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2018

Mark-Simulacrum requested changes Apr 26, 2018

View reviewed changes

collin5 force-pushed the b48483 branch from 4e8f4ed to 281f534 Compare April 27, 2018 10:11

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 1, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 7, 2018

collin5 commented May 7, 2018

View reviewed changes

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 8, 2018

collin5 force-pushed the b48483 branch from 1fec0d9 to bd8f2a0 Compare May 8, 2018 13:12

kennytm mentioned this pull request May 8, 2018

Rollup of 11 pull requests #50546

Merged

ignore test-args if user specifies suite_path

2f8c2a9

collin5 force-pushed the b48483 branch from bd8f2a0 to 2f8c2a9 Compare May 9, 2018 00:24

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 9, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2018

bors merged commit 2f8c2a9 into rust-lang:master May 10, 2018

collin5 deleted the b48483 branch May 12, 2018 02:29

ollie27 mentioned this pull request Feb 15, 2020

Fix running rustdoc-js test suite individually #69198

Merged

./x.py test should be able to run individual tests #49729

./x.py test should be able to run individual tests #49729

Conversation

collin5 commented Apr 6, 2018 • edited by Mark-Simulacrum Loading

rust-highfive commented Apr 6, 2018

alexcrichton commented Apr 6, 2018

collin5 commented Apr 6, 2018 • edited Loading

collin5 commented Apr 13, 2018

alexcrichton commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-highfive commented Apr 15, 2018

collin5 commented Apr 15, 2018

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 18, 2018

collin5 commented Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

collin5 commented Apr 27, 2018

shepmaster commented May 6, 2018

Mark-Simulacrum commented May 7, 2018

bors commented May 7, 2018

rust-highfive commented May 7, 2018

collin5 May 7, 2018 • edited Loading

Choose a reason for hiding this comment

rust-highfive commented May 8, 2018

Mark-Simulacrum commented May 8, 2018

bors commented May 8, 2018

collin5 commented May 9, 2018 • edited Loading

Mark-Simulacrum commented May 9, 2018

bors commented May 9, 2018

bors commented May 10, 2018

bors commented May 10, 2018

collin5 commented Apr 6, 2018 •

edited by Mark-Simulacrum

Loading

collin5 commented Apr 6, 2018 •

edited

Loading

collin5 commented Apr 24, 2018 •

edited

Loading

collin5 May 7, 2018 •

edited

Loading

collin5 commented May 9, 2018 •

edited

Loading