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

Placate Clippy #126

Merged
merged 4 commits into from
Oct 27, 2021
Merged

Placate Clippy #126

merged 4 commits into from
Oct 27, 2021

Conversation

itowlson
Copy link
Contributor

No description provided.

@radu-matei
Copy link
Member

radu-matei commented Oct 18, 2021

I'm getting a test failure on macOS:

thread 'test::can_serve_multiple_static_entrypoints' panicked at 'assertion failed: `(left == right)`
  left: `200`,
 right: `404`', src/lib.rs:155:9

In fact, there seem to be a flaky test somewhere.

---- test::can_serve_from_modules_toml stdout ----
No log_dir specified, using temporary directory /var/folders/rs/7g5937rn0sq1vyrtwf0_48140000gn/T/.tmpXi9eIx for logs
thread 'test::can_serve_from_modules_toml' panicked at 'assertion failed: `(left == right)`
  left: `200`,
 right: `404`', src/lib.rs:233:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test::can_serve_from_modules_toml

@itowlson
Copy link
Contributor Author

Well Butcher should have bought me a M-- oh wait

@itowlson
Copy link
Contributor Author

Two possibilties:

  1. When it tries to sub an absolute path into the test*.toml - something is going amiss between Mac and file: URL formats. I doubt this because it is passing in CI, but it's possible.

  2. There was also a concurrency bug in the way replace_placeholders created temporary files, which is fixed in a different PR. This would result in flakes. I'm not sure if there's a way to run tests single threaded...

@itowlson
Copy link
Contributor Author

The fix for the concurrency bug is to change let tempfile_path = tempfile_dir.join("modules.toml"); in replace_placeholder to pass original_map_file instead of "modules.toml".

@radu-matei
Copy link
Member

Ok, given the concurrency bug is solved somewhere else, this PR LGTM.

Thanks!

@itowlson itowlson merged commit abecd21 into deislabs:main Oct 27, 2021
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

2 participants