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

Tests no longer require internet #300

Merged
merged 3 commits into from
Dec 3, 2020
Merged

Tests no longer require internet #300

merged 3 commits into from
Dec 3, 2020

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Nov 23, 2020

This makes them more hermetic and makes the tests less flaky and makes porting this project to build with Bazel easier.

The changes in this PR add metadata templates to impl/src/testing/metadata_templates which are the formatted outputs of cargo metadata --format-version 1. Each .template file has the Cargo.toml contents at the top of it that produced it. These templates are then rendered during the relevant tests (test similar to the file name). This avoids having to fetch any metadata from the internet during testing.

This change also adds RazeMetadataFetcher which is the the new class to use for gathering metadata for the planning phase of cargo-raze. Note that this no longer implements MetadataFetcher as MetadataFetcher is intended to be a wrapper around a basic way to generate a cargo_metadata::Metadata object. RazeMetadataFetcher is intended to generate a RazeMetadata object.

This PR is blocked by #298

@UebelAndre
Copy link
Collaborator Author

@acmcarther For this one I wasn't quite sure what to do with the metadata. I opted to store it and try to keep changes to code minimal but it would make sense to change the crates used in some of these tests.

@UebelAndre
Copy link
Collaborator Author

Also worth noting, this only has as many new lines of code because the metadata dumps are pretty printed but I feel that's the correct thing to do as it makes it more readable and makes it easier to track the footprint of that metadata.

…taCommand` when the host had no network connectivity
@UebelAndre UebelAndre marked this pull request as ready for review November 25, 2020 19:18
@UebelAndre
Copy link
Collaborator Author

@acmcarther This is rebased and ready for review now 🙏

Copy link
Member

@acmcarther acmcarther left a comment

Choose a reason for hiding this comment

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

I'm not sure how I feel about the templated json, but I think it's clearly better than having the tests depend on fetching stuff from the internet.

Generally LGTM, mostly nits. High level nit as well: Can you update the PR description here to describe the abstractions you're adding and the motivation for adding them? That practice makes it easier to digest medium and larger sized PRs.

impl/src/metadata.rs Outdated Show resolved Hide resolved
impl/src/metadata.rs Outdated Show resolved Hide resolved
impl/src/planning.rs Show resolved Hide resolved
@UebelAndre
Copy link
Collaborator Author

@acmcarther how's the description look now? Also, still stuck on the actix-web test

@UebelAndre
Copy link
Collaborator Author

I'm not sure how I feel about the templated json, but I think it's clearly better than having the tests depend on fetching stuff from the internet.

@acmcarther I'm not in love with it either but I do feel it's better than what was before. If there's a simpler solution then I could take another pass. But I do feel this change would be good to get us on the path to something better.

@UebelAndre
Copy link
Collaborator Author

@acmcarther Hey, are you around today? 🙏 😅

@acmcarther
Copy link
Member

acmcarther commented Dec 2, 2020

You're doing that force push thing again which is breaking the diff links.

To be really clear the thing that is broken is clicking file links for prior comments, such as this: https://github.com/google/cargo-raze/pull/300/files/f7c7512837473503fcab89c08d4180968c53e43b#diff-1fb1e871b8f9024bde534a1e34e1b0285f4712c79a01284605eb55cf6e7a3e36

@acmcarther
Copy link
Member

acmcarther commented Dec 2, 2020

I'm ready to merge this once the open question about that test which used actix-web is answered. Sorry for the slowness on the review.

Re: description: The new contents of the description are helpful. I was being a bit more literal with my request to include the abstractions -- i meant to literally include the non-trivial new structs. I don't feel strongly about that, hence I didn't raise that. What we have here looks good to me.

@UebelAndre
Copy link
Collaborator Author

I'm ready to merge this once the open question about that test which used actix-web is answered. Sorry for the slowness on the review.

Re: description: The new contents of the description are helpful. I was being a bit more literal with my request to include the abstractions -- i meant to literally include the non-trivial new structs. I don't feel strongly about that, hence I didn't raise that. What we have here looks good to me.

@acmcarther I can do that from here on. I think this is my last large PR though since I feel all the large missing pieces of functionality are in.

You're doing that force push thing again which is breaking the diff links.

To be really clear the thing that is broken is clicking file links for prior comments, such as this: https://github.com/google/cargo-raze/pull/300/files/f7c7512837473503fcab89c08d4180968c53e43b#diff-1fb1e871b8f9024bde534a1e34e1b0285f4712c79a01284605eb55cf6e7a3e36

Sorry, I forgot that this PR was in review when fixing up my branches. I am making an effort to not force push once a PR is in review. Since all my PRs from here on are smaller, I think this will get easier.

@UebelAndre UebelAndre mentioned this pull request Dec 2, 2020
4 tasks
@UebelAndre
Copy link
Collaborator Author

@acmcarther I've also added more to the description.

@UebelAndre
Copy link
Collaborator Author

@acmcarther Hey, are you around today to take another look here? 🙏

@acmcarther acmcarther merged commit f209e9f into google:master Dec 3, 2020
@UebelAndre
Copy link
Collaborator Author

Thank you so much! 🙏

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