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

Fix load_from_json #137

Merged
merged 3 commits into from Jan 19, 2020
Merged

Fix load_from_json #137

merged 3 commits into from Jan 19, 2020

Conversation

CasperWA
Copy link
Member

This function did not work properly.

Add test file for config.py, for now only with a simple test to run load_from_json().

Feel free to add more tests to test_config.py either in this PR or later.

Add test for config.py, for now only with a simple test to run
load_from_json().
@CasperWA CasperWA added the bug Something isn't working label Jan 16, 2020
@codecov
Copy link

codecov bot commented Jan 16, 2020

Codecov Report

Merging #137 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #137   +/-   ##
=======================================
  Coverage   85.91%   85.91%           
=======================================
  Files          39       39           
  Lines        1818     1818           
=======================================
  Hits         1562     1562           
  Misses        256      256
Flag Coverage Δ
#unittests 85.91% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df73643...e9d1c98. Read the comment docs.

@CasperWA CasperWA mentioned this pull request Jan 17, 2020
@ml-evs
Copy link
Member

ml-evs commented Jan 18, 2020

Thanks for this @CasperWA!

I've just added the same test on our default config.ini, guess its a bit redundant as its being tested indirectly everywhere else, but oh well!

I'll approve this and you can merge, unless you see any issues with my changes.

@CasperWA
Copy link
Member Author

I've just added the same test on our default config.ini, guess its a bit redundant as its being tested indirectly everywhere else, but oh well!

I'll approve this and you can merge, unless you see any issues with my changes.

Don't see any problems, only - why not use the actual config.ini file for these tests? Then we'll also know if we do something wrong with that one :)

@ml-evs
Copy link
Member

ml-evs commented Jan 18, 2020

I've just added the same test on our default config.ini, guess its a bit redundant as its being tested indirectly everywhere else, but oh well!
I'll approve this and you can merge, unless you see any issues with my changes.

Don't see any problems, only - why not use the actual config.ini file for these tests? Then we'll also know if we do something wrong with that one :)

Could do, I just figured having a separate test file might be helpful if we make any changes to the default. I'll leave it up to you!

@CasperWA CasperWA merged commit 1ff9d52 into master Jan 19, 2020
@CasperWA CasperWA deleted the fix_load_from_json branch January 19, 2020 13:14
@CasperWA CasperWA mentioned this pull request Feb 5, 2020
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants