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

uucore: Start testing uucore #2030

Merged
merged 2 commits into from
Apr 5, 2021

Conversation

marvin-bitterlich
Copy link
Contributor

Before this change we never ran tests on uucore itself
meaning that is was not possible to test
functions of the shared core, only their usage
in the different binaries

This change adds running uucore to our ci, which will increase coverage for the few doctests that exist

and is extracted from #1988 where first tests for uucore will be introduced

Before this change we never ran tests on uucore itself
meaning that is was not possible to test
functions of the shared core, only their usage
in the different binaries

This change adds running uucore to our ci, which will increase coverage for the few doctests that exist

and is extracted from uutils#1988 where first tests for uucore will be introduced
@sylvestre
Copy link
Contributor

This is great, thanks

@marvin-bitterlich
Copy link
Contributor Author

This is great, thanks

I tried for hours to introduce it into the main test pipeline, but that was blocked by cargo not exposing features of dependencies

So I could not conditionally enable tests for uucore/fs because that feature is conditionally enabled depending on which binaries you build, say rm might enable uucore/fs, but since the tests don't run inside the uucore cargo project, cargo does not expose if that flag is enabled or not.

Any solution that tries to hardcode the binaries that enable certain uucore features would be brittle and prone to mistakes, so I decided to bite the bullet and just make sure that we test our core crate as its own thing.

From my tests this supports our coverage tooling that is currently used and should incentivize more throughout testing

@marvin-bitterlich
Copy link
Contributor Author

Pygments requires Python '>=3.5' but the running Python is 2.7.12
You are using pip version 19.0.3, however version 21.0.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
The command "if [ $TRAVIS_OS_NAME = linux ]; then sudo apt-get install python-pip && sudo pip install $SPHINX_VERSIONED; fi" failed and exited with 1 during .

The failed travis check seems unrelated to my code

@sylvestre
Copy link
Contributor

I don't see the new uucore jobs in the list of checks, is that expected?

@marvin-bitterlich
Copy link
Contributor Author

I don't see the new uucore jobs in the list of checks, is that expected?

I added them as part of the other tests, so that whenever we run tests we also run the uucore tests related to the code, that way if a binary enables a feature we run the tests of that utility and its uucore tests.

I felt that is the more holistic approach

@@ -11,4 +11,4 @@ task:
- cargo build
test_script:
- . $HOME/.cargo/env
- cargo test
- cargo test -p uucore -p coreutils
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically implicitly we always run cargo test -p coreutils and by specifying to also include the project -p uucore we test more things in the same run

It is important to run both at the same time, because otherwise the feature flags enabled by compiling coreutils are not passed to uucore.

As an example, the uucore/fs tests are only ran if anything in coreutils enables the fs feature. Which this we piggyback upon the wide array of tests and conditional features being tested for all the platforms they are valid on and don't have to develop a second set of feature flags for uucore itself

@sylvestre sylvestre merged commit 9e0ac37 into uutils:master Apr 5, 2021
@marvin-bitterlich marvin-bitterlich deleted the test-coreutils branch April 5, 2021 20:28
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.

2 participants