Skip to content

Test load_config() #1396

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 7, 2022
Merged

Test load_config() #1396

merged 4 commits into from
Oct 7, 2022

Conversation

@nertpinx
Copy link
Contributor

Few patches for things I found when trying to add tests for load_config().

Closes #345

@nertpinx
Copy link
Contributor Author

I marked this as a draft since I am not sure this is the proper way of contributing. Let me know if any changes are needed. Thanks.

@codecov
Copy link

codecov bot commented Sep 15, 2022 •
edited
Loading

Codecov Report

Merging #1396 (0446cd1) into develop (b2b2a80) will decrease coverage by 0.93%.
The diff coverage is n/a.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1396      +/-   ##
===========================================
- Coverage    66.19%   65.26%   -0.94%     
===========================================
  Files           86       81       -5     
  Lines         9068     8767     -301     
===========================================
- Hits          6003     5722     -281     
+ Misses        3065     3045      -20

j-c-cook
j-c-cook reviewed Sep 17, 2022
View reviewed changes
setup.cfg Outdated
@@ -1,5 +1,5 @@
[metadata]
license_file = LICENSE.txt
license_files = LICENSE.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is explained in the commit message which is not immediately seen in GitHub, I can split it into a separate PR, just like the other unrelated patches.

8e9b8fc

Copy link
Contributor

Choose a reason for hiding this comment

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

It is good to keep PR’s within the narrowest scope as practical. Perhaps a quote by Einstein somewhat applies; “Everything should be made as simple as possible, but not simpler.” The review process is simpler (and often can occur quicker) if changes fall within a scope described in the related issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I removed the other patches and will post them separately.

@j-c-cook
Copy link
Contributor

I marked this as a draft since I am not sure this is the proper way of contributing. Let me know if any changes are needed. Thanks.

Your contribution workflow looks good. Once you think this is ready to go in, mark it as ready for review.

You’ll need to format your code with the black package. First install the linters requirements, pip install -r requirements-lint.txt and then run black —verbose . in the root directory of python-can. This should auto format the 3 files that are causing the formatting check to break.

@nertpinx nertpinx marked this pull request as ready for review September 18, 2022 08:44
@nertpinx
listener_test: Do not modify environment for other tests
acbc53b
@nertpinx
network_test: Do not modify environment for other tests
dd581a6 
Also do not skip the test since there will always be an interface (previously
set globally and now prepared in setUp function.
@nertpinx
test_load_file_config: Test with different values
559c105 
Testing with the same interface values would not show possible issues in case
there were any.
@nertpinx
tests: Add test for load_config
0446cd1 
Add tests for load_config with contexts and environment variables.

Closes hardbyte#345

Signed-off-by: Martin Kletzander <nert.pinx@gmail.com>
hardbyte
hardbyte approved these changes Sep 30, 2022
View reviewed changes
Copy link
Owner

@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

Thanks for adding these tests (and your contributions across other PRs)

@zariiii9003 zariiii9003 merged commit a144f25 into hardbyte:develop Oct 7, 2022
@nertpinx nertpinx deleted the test_load_config branch October 10, 2022 14:39
j-c-cook added a commit to j-c-cook/python-can that referenced this pull request Nov 11, 2022
@j-c-cook
Revert "Test load_config() (hardbyte#1396)"
149a20c 
This reverts commit a144f25.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hardbyte hardbyte hardbyte approved these changes

+1 more reviewer

@j-c-cook j-c-cook j-c-cook left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add tests for load_config()

4 participants

@nertpinx @j-c-cook @hardbyte @zariiii9003