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

[Issue 21] Migrate to pytest #30

Merged
merged 17 commits into from
Oct 12, 2017
Merged

[Issue 21] Migrate to pytest #30

merged 17 commits into from
Oct 12, 2017

Conversation

djrtwo
Copy link
Collaborator

@djrtwo djrtwo commented Oct 11, 2017

What was wrong

Issue #21
Issue #28

How was it fixed

  • Installed pytest. Added to requirements.txt
  • Migrated each test_* file to pytest format
  • Parametrize tests to increase code reuse
  • add "--report" flag to enable plotting on tests
  • reorganize src files into /casper (issue Organize source files into sub directory #28)

from testing_language import TestLangCBC


@pytest.mark.parametrize(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@naterush We can parametrize all of the tests in this file in the following format. On the one hand, it is great for code reuse. On the other, it makes it a little more nebulous as to what the crazy test string relates to (going from having a function name to just a comment defining it).

Thoughts on porting the rest of the tests in this file to this format?
I'm not sure which I prefer

Copy link
Collaborator

Choose a reason for hiding this comment

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

@djrtwo hmm, good question. I don't feel particularly strongly either way, but am kinda leaning to keeping them as separate tests.

The code duplication we could get rid up would probably be counter-balanced by the extra comments we would need to explain each test. I experimented by moving the all safety oracle tests to the parameterized format, and things got dense and unreadable pretty quickly.

This works super great for testing utils though - just don't think it's the best for the test language :) Let me know what you think.

@djrtwo
Copy link
Collaborator Author

djrtwo commented Oct 11, 2017

@naterush @zramsay Ready for review. Still need to add flag for enabling showing plots.

@zramsay
Copy link
Collaborator

zramsay commented Oct 11, 2017

readme should be updated; not sure what the exact pytest ... command should be

@djrtwo
Copy link
Collaborator Author

djrtwo commented Oct 11, 2017

@zramsay readme updated

@zramsay
Copy link
Collaborator

zramsay commented Oct 11, 2017

👍 works great, tests pass

@naterush
Copy link
Collaborator

naterush commented Oct 11, 2017

@djrtwo this looks great - should be good to merge! I removed the parametrized version of the testing_language stuff based on comments above. Happy to add back/move rest of tests to that format if we decide it's better. Let me know.

I'm going to add an issue about the errors rather than adding it here so we can discuss further.

@naterush
Copy link
Collaborator

Ah, let's add command line option for toggling reporting before merging this.

@djrtwo
Copy link
Collaborator Author

djrtwo commented Oct 12, 2017

@naterush @zramsay

  • Tests pass on circleci
  • added "--report" to plot during tests
  • reorganized source files out of root dir and into /casper

Take a look for final review

@djrtwo
Copy link
Collaborator Author

djrtwo commented Oct 12, 2017

kernprof -l casper.py [] is broken due to the re-org. Fixing now

@djrtwo
Copy link
Collaborator Author

djrtwo commented Oct 12, 2017

fixed. ready for review

Copy link
Collaborator

@naterush naterush left a comment

Choose a reason for hiding this comment

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

Looks great. Things are starting to get real legit :~). Merging now!

@naterush naterush merged commit 3ef1f0d into blockchain Oct 12, 2017
@naterush naterush deleted the chore/migrate-to-pytest branch October 12, 2017 17:42
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.

None yet

3 participants