-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add _from
variants to support threaded tests
#23
Conversation
Hmm, missed a site, updating it... |
These permit abstracting over some common global process state and allow the use of home in project where in-process threaded tests are desirable, and where rustup / cargo state is changed during the tests.
All verified with my branch of rustup that is doing in-process testing. |
Sorry for late reply! Overall I feel like this patch should remain in rustup. |
Hi @lzutao without this patch we will have to stop using this crate. For now we've moved to using a forked copy of it. Yes, the motivation for the patch is to facilitate testing, but because the state that home consults is process global state, we need a trait to influence it to be able to perform threaded tests in-process. Cargo and other tools would be able to write in-process threaded tests using this facility as well. We haven't yet taken full advantage of this capability - our interlocks to deal with process invocation file locking races are still present - but even with that in place we've gotten ~8% test improvement with this, and are targeting much higher numbers as we can now move to unit tests rather than full stack tests, without having to invert the entire code base to introduce explicit dependency injection everywhere. (And even if we did that, we'd still have to modify home to permit injection into home for the state it consults). |
Another way of thinking about this is as follows: imagine that rustup had implemented this in-process threaded testing support before this library was broken out to unify the code. We would still have wanted to do the unification of the domain logic, but supporting this testing feature would not be a matter of question or debate. So I don't think it should be now either. The how can certainly be - if you have different interfaces or means you'd like to see, just let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry for taking long to get back here.
This PR was on my TODO list. I kept ignoring it for several reasons.
Part of that is that I really don't really get
what problems the new added API going to solve.
You said about in-process thread tests. What are they?
Do they run rustup binary in each threaded of integration tests?
The rustup code that use the new APIs?
The second part is that why rustup couldn't add a wrapper around "home" crate API
for #[cfg(test)]
only that could be used by unit tests?
That's said. Not receiving enough context makes me to be not confident about
adding new APIs and so I kept ignoring this PR.
PS: I don't satisfy with current new API documentation. But that another problem
before I clear out my misunderstandings first.
PS: Opensource is not easy. I will try my best to make API useful, easy-to-use.
So please bear with me.
One reason for not wanting to special-case the operations in our test suite is that it'd alter the logic for handling our detection logic since we'd essentially have to special-case it in the test suite instead of trusting that the same logic will be run against our mocked environment. If we were to start putting down locks and manipulating the environment instead then it'd cause problems for any other unlocked access to the environment, and locking all access to the mocked environment would partially defeat the point of permitting more parallel in-process testing (to speed up our test cycle) I can understand your reticence to increase your API surface, but I feel this addition will be helpful and not complex to maintain. Also I, and I imagine @rbtcollins would be prepared to help maintain things if you are very busy. We did briefly discuss suggesting that bringing |
Hi @lzutao the challenge we have is that the rustup test suite is very slow; it is very slow because many of our tests involve forking and running an entire process, often many times. To fix this and have unit tests is going to be a very large amount of work for 2 reasons. and two, because we need to go through the code and figure out what each test that current invokes the CLI is actually trying to test, rewrite it as a narrow layer specific test, figure out if that reduces test coverage, if so backfill other narrow tests for the things left uncovered and so on. This patch is in support of step one. There are basically two ways that dependency injection can be done: via thread local state, or via passed-down-state. The code churn to do passed-down-state is many thousands of lines of code change in rustup: we have to move the lookup of home directory, current working directory, username - everything like that - out of the library and into the top level entry point, store it in some sort of state struct, and then pass that down into every single function through the entire code base. It is doable but it makes the entire code base vastly more noisy and hard to read. We tried that first. To see our in-process test support, have a look in the rustup code base; as I said, we've save about 8% on test runtime already simply by not A test only wrapper would be we have to compile rustup twice during tests, and @kinnison is already unhappy with compile times; secondly it would as @kinnison says reduce confidence in our test results. As it is we can inspect https://github.com/rust-lang/rustup/blob/a93284b929d2d6716f8bedebcbf3658b09763e9c/src/utils/utils.rs#L487 and see that it is the same code path in test or release builds. |
This needs to be public to allow rustup to cleanly call the from family of functions but still benefit from the Windows specific codepath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review again! My remote build machine has died recently and I had a long time to set it up again.
I let a few thoughts. Most are just nitpicking.
That's really nice to see your interests. I help maintain things but I don't have ownership of this repository. |
We'd only be doing this if between @brson and @lzutao maintenance is not really viable. Given how busy you both seem to be, it might be worthwhile doing, and putting ownership into the dev-tools group which I think @lzutao is a member of anyway. What do you both think? If you'd rather keep control then I have no objections. |
FYI this is partly blocking rustup from being packaged in distros, since rustup is depending on a specific commit on this PR branch, and distros are unlikely to package this on its own. It's sad to read the disagreement over the default implementation for My (potentially ignorant, since I knew nothing about this crate) opinion is that the current state (no default for |
This PR follows up and replaces on #23 which intended to add mockable functions to the `home` crate. This PR follows in similar vain except for a few changes. - All mockable fn and the `Env` trait moved into a new `env` module to keep the docs clean from all the extra methods that are usually not required for most users of the crate. - Rename the functions to be `with_env` instead of just having `_from`. The goal of this PR is to enable `rustup` and hopefully `cargo` in the future to be able to write tests which mockout the environment while still going through the core logic of this crate.
Closing this in favor of #29 |
These permit abstracting over some common global process state and allow
the use of home in project where in-process threaded tests are
desirable, and where rustup / cargo state is changed during the tests.