Simplify mocking IO in PackageManager, add a test for add-path
#2881
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ PR OK, no changes in deprecations or warnings Total deprecations: 0 Total warnings: 0 Build statistics: statistics (-before, +after)
executable size=5356064 bin/dub
-rough build time=63s
+rough build time=62sFull build output |
Geod24
force-pushed
the
mlang/Test2481
branch
from
aefb2cb
to
ff06a7a
Compare
Moving it to PackageManager allows us to call PackageManager.existsFile, allowing us to mock a lot more things.
This allows us to test the behavior of the base class much better, by overriding the last IO function, readText.
This is one more step towards removing our mocking of refresh_.
We can finally test the full features of the base function, as we are now mocking the IO, not the logic.
Mocking IO is required for writeLocalPackageList, as we want to test the 'add-path' behavior. While it is not a big concern for overrides, as they are deprecated and we are unlikely to add new tests, it is easy to do, and already done for reading them.
This will prove handy when writting tests that require multiple invocation of Dub, such as idempotence tests, or things like add-path which currently don't propagate their change through Dub as they are one shot commands.
So that tests can use 'NativePath' and 'toNativeString'.
This was not currently tested explicitly, and is currently the reason why we still have the scanning behavior in the PackageManager. In order to remove the scanning behavior and have the PM lazily scan packages, we first must add test to ensure there will be no further regression.
Geod24
force-pushed
the
mlang/Test2481
branch
from
ff06a7a
to
1082f7f
Compare
|
This has quite a lot of improvements I need to fix other bugs (write more tests), so I'm going to merge it in ~10 hours unless anyone objects. |
thewilsonator
approved these changes
Feb 26, 2024
PetarKirov
reviewed
Feb 28, 2024
There was a problem hiding this comment.
I reviewed the commits one-by-one and everything looks moving in the right direction. Awesome work @Geod24 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@s-ludwig : I would like to remove the scanning behavior in
PackageManager. This is a source of pain for us, as it slows down builds, including with @atilaneves Reggae.Last time that introduced a regression which you fixed in #2481 .
This simplifies mocking quite a bit and introduces a test for it (see last commit /
source/dub/test/other.din the diff).If you could have a quick look at the test and confirm that it covers your use case ?