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

Add --no-config option #1896

Merged
merged 4 commits into from Jul 1, 2023
Merged

Add --no-config option #1896

merged 4 commits into from Jul 1, 2023

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented Jul 1, 2023

Addresses #1863 (comment).

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@atugushev atugushev added enhancement Improvements to functionality cli Related to command line interface things labels Jul 1, 2023
dry_run_message = "Would install:"
assert dry_run_message in out.stdout

out = runner.invoke(cli, ["--no-config", "--config", config_file.as_posix()])
Copy link
Member

Choose a reason for hiding this comment

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

This should be a separate test with its own name or at least a parametrized case..

Copy link
Member Author

@atugushev atugushev Jul 1, 2023

Choose a reason for hiding this comment

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

Generally, I agree, however in this case, I prefer the triangulation technique.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how it's applicable in this case. You're clearly testing two separate behaviors that must appear as separate entries in the test report.
The only way of having two things reported from the same test function is to use pytest-subtest. But it still feels wrong since you're not checking two properties of the same behavior outcome.

Copy link
Member

Choose a reason for hiding this comment

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

Also, for property-based testing, we should really integrate Hypothesis..

Copy link
Member Author

@atugushev atugushev Jul 1, 2023

Choose a reason for hiding this comment

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

Okay, here you go 65fe40f

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Let's try making the test names reflect what it is we're testing...

tests/test_cli_compile.py Outdated Show resolved Hide resolved
tests/test_cli_sync.py Outdated Show resolved Hide resolved
atugushev and others added 2 commits July 1, 2023 12:21
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
@atugushev atugushev enabled auto-merge (squash) July 1, 2023 10:40
@atugushev atugushev merged commit 407f008 into jazzband:main Jul 1, 2023
36 checks passed
@atugushev atugushev deleted the no-config branch July 1, 2023 11:50
@atugushev atugushev added this to the 7.0.0 milestone Jul 14, 2023
@atugushev atugushev added the config Related to pip-tools' configuration label Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to command line interface things config Related to pip-tools' configuration enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants