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

Refactoring tests to its specific tasks #58

Closed
anaschwendler opened this issue May 11, 2017 · 11 comments
Closed

Refactoring tests to its specific tasks #58

anaschwendler opened this issue May 11, 2017 · 11 comments

Comments

@anaschwendler
Copy link
Collaborator

anaschwendler commented May 11, 2017

This issue aims at refactoring two existent tests that are working well, but need enhancements to keep tests independent of network connection and writing to/reading filesystem.

Tests that need to be refactored:
tests/test_federal_senate_dataset.py
tests/test_chamber_of_deputies_dataset.py

@cuducos would like to help in it :)

@cuducos
Copy link
Collaborator

cuducos commented May 11, 2017

Probably next week I can handle that. If anyone wants to pair on that, drop a line saying day/time it could work for you.

@cuducos
Copy link
Collaborator

cuducos commented May 16, 2017

cc @lipemorais

@cuducos
Copy link
Collaborator

cuducos commented May 16, 2017

Notes by @lipemorais in #53 about def __translate_file(self, csv_path):

This internal method looks a little big, could we break it to make the parts smaller and more meaningful and clear?

At this line looks that is a translation from portuguese to english but it's changing state of categories variable here. It is making this code more complex than it could be. :/

May it could be done in a function just to give it a name and help with clarity.
Show all reviewers

(this last comment about line 93)

@anaschwendler
Copy link
Collaborator Author

We we're thinking about testing:

  • Check the translations
  • The clean() method unify all the senate datasets, maybe test if the reimbursements are getting concatenated (this is against TDD, but ¯_(ツ)_/¯ )
  • The fetch() method does not check if the senate website is available to fetch the datasets, maybe test it or implement something should be good :)

@cuducos
Copy link
Collaborator

cuducos commented May 16, 2017

this is against TDD, but

In Brazilian Portuguese TDD means teste depois do deploy, right?

@lipemorais
Copy link
Contributor

lipemorais commented May 16, 2017

I'm run the tests and I just realized one thing, because it's taking more time than I expect.

Test this PR would even easier with unit tests. Just fetch needs integration, translate and clean could use only use unit test with a example files. :/

@anaschwendler
Copy link
Collaborator Author

@lipemorais Yes, I was talking about that tests taking so long to run yesterday in your daily meeting, and I didn't know what to do, do you have any suggestion for us? Something like a light or something? I really doesn't know what to do :T

I just really tested what I wanted to do, and didn't know how to properly refactor it

@lipemorais
Copy link
Contributor

lipemorais commented May 16, 2017

I can think now in two ways:

  1. I would isolate the integration point, in this case is fetch and unit test translate and clean with a mocked example files.

  2. In a second way I would unit test the three steps using mock while transform the integration test into a journey test, that should test this workflow without any mocks and let it run in Travis CI.

@lipemorais
Copy link
Contributor

I'm working on it. :)

@lipemorais
Copy link
Contributor

Hey!
Do we consider it half done with the PR #68, now it should be done for CEAP also.

@anaschwendler
Copy link
Collaborator Author

Ok, I'll close it ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants