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

Mach test cargo target dir #27169

Merged
merged 1 commit into from Jul 4, 2020

Conversation

alarsyo
Copy link
Contributor

@alarsyo alarsyo commented Jul 3, 2020


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because they're about tooling

This change allows launching tests this way:

CARGO_TARGET_DIR=other-dir ./mach test-wpt ...

The build and check commands already check for the presence of the CARGO_TARGET_DIR variable, but that wasn't the case for wpt tests. This resulted in an error about the servo executable not being found in target/debug/servo.

@highfive
Copy link

highfive commented Jul 3, 2020

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

@highfive
Copy link

highfive commented Jul 3, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/document.rs, components/script/script_thread.rs, components/script/dom/window.rs
  • @jgraham: tests/wpt/run.py
  • @KiChjang: components/script/dom/document.rs, components/script/script_thread.rs, components/script/dom/window.rs

@alarsyo
Copy link
Contributor Author

alarsyo commented Jul 3, 2020

Woops, didn't create the branch from master. Let me rebase this.

@alarsyo alarsyo force-pushed the mach-test-cargo-target-dir branch from aad0de3 to bcd6ef8 Compare Jul 3, 2020
@jdm
Copy link
Member

jdm commented Jul 3, 2020

@bors-servo r+
Thanks for finding and fixing this!

@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

📌 Commit bcd6ef8 has been approved by jdm

bors-servo added a commit that referenced this issue Jul 3, 2020
Mach test cargo target dir

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [X] These changes do not require tests because they're about tooling

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

This change allows launching tests this way:

```sh
CARGO_TARGET_DIR=other-dir ./mach test-wpt ...
```

The build and check commands already check for the presence of the `CARGO_TARGET_DIR` variable, but that wasn't the case for wpt tests. This resulted in an error about the `servo` executable not being found in `target/debug/servo`.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

Testing commit bcd6ef8 with merge f7ff1d7...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

💔 Test failed - status-taskcluster

tests/wpt/run.py Outdated
@@ -72,7 +73,10 @@ def set_defaults(kwargs):
bin_name = "servo"
if sys.platform == "win32":
bin_name += ".exe"
bin_path = servo_path("target", bin_dir, bin_name)
if os.environ["CARGO_TARGET_DIR"]:
Copy link
Member

@jdm jdm Jul 3, 2020

Choose a reason for hiding this comment

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

Looks like we need os.environ.get("CARGO_TARGET_DIR") instead.

Copy link
Contributor Author

@alarsyo alarsyo Jul 3, 2020

Choose a reason for hiding this comment

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

My bad, didn't test without the variable :) should be fixed!

@jdm
Copy link
Member

jdm commented Jul 3, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

📌 Commit 9f224c4 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

Testing commit 9f224c4 with merge e889d1b...

bors-servo added a commit that referenced this issue Jul 3, 2020
Mach test cargo target dir

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [X] These changes do not require tests because they're about tooling

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

This change allows launching tests this way:

```sh
CARGO_TARGET_DIR=other-dir ./mach test-wpt ...
```

The build and check commands already check for the presence of the `CARGO_TARGET_DIR` variable, but that wasn't the case for wpt tests. This resulted in an error about the `servo` executable not being found in `target/debug/servo`.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

💔 Test failed - status-taskcluster

@bors-servo
Copy link
Contributor

bors-servo commented Jul 4, 2020

Testing commit 9f224c4 with merge 026760a...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 4, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 026760a to master...

@bors-servo bors-servo merged commit 026760a into servo:master Jul 4, 2020
2 checks passed
@alarsyo alarsyo deleted the mach-test-cargo-target-dir branch Jul 4, 2020
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.

None yet

4 participants