fix: remove test code from neard #4333
Closed
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
testlib is the library we use for testing, it makes no sense that we compile it into our production code. Admittedly, I lack the context as to *why* it was added as a non-dev dep in the first place. I have a suspicion that it was a workaround some feature bugs. Now that we use resolever=2, it makes sense to try to fix this in a more principled way, so let's see if some tests start failing due to this change.
|
These two tests failed, related to nightly protocol features: |
frol
approved these changes
May 28, 2021
|
#4339 does this in a better way I think! |
near-bulldozer bot
pushed a commit
that referenced
this pull request
Jun 2, 2021
We're sort of in a weird spot as it is, we have features within `nearcore` that are required but can't be enabled if they depend on crates listed under `dev-dependencies` (bug in cargo: rust-lang/cargo#6915). Temporarily listing them under `dependencies`, fixed the CI testing failures we had while working on #4292 (same error @ailisp highlighted: #4333 (comment)), but as pointed out in #4331 (comment), this method should be out of the question. If we remove it, however, we can't work with the tests that depend on those features. This PR moves the _previously top-level_ tests into a new crate to have better dependency and feature handling as @matklad suggested > * [move tests from `/test` folder somewhere -- either to some of the existing packages (neard perhaps?) or into a new `integration-tests` package](#4292 (comment)) This would ensure we have dependencies like `testlib`, `runtime-params-estimator` and `restored-receipts-verifier` that are only needed for testing and depend on nearcore without cyclic dependency issues while having all features relevant to testing in order. The features within `neard` have been reduced to proxies to `nearcore` features, which partially resolves #4325 (though the intent for that has morphed a bit). This also makes for a cleaner dependency graph from `neard` perspective, removing extraneous dependencies that were previously required. I also noticed removed the `old_tests` feature that should've been removed along with the rest of it in #928. Updated some docs and code comments referencing the old `neard` path too.
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.
testlib is the library we use for testing, it makes no sense that we
compile it into our production code. Admittedly, I lack the context as
to why it was added as a non-dev dep in the first place. I have a
suspicion that it was a workaround some feature bugs.
Now that we use resolever=2, it makes sense to try to fix this in a more
principled way, so let's see if some tests start failing due to this
change.