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 first reccard test #77

Merged
merged 9 commits into from
Jul 3, 2020
Merged

Fix first reccard test #77

merged 9 commits into from
Jul 3, 2020

Conversation

cydanil
Copy link
Collaborator

@cydanil cydanil commented May 16, 2020

This PR aims to close #32, where we fix the tests for the record card.

These are more of integration tests, ranging from GUI action, all the way through the widgets, and checking that the data to be stored matches the inputs.

I apologise if the first commit seems so huge: it's the culmination of all PRs I've done on this project thus far.

It's still a work in progress, but I'm proud to say that the first test passes!

Since it's been a long way, I would recommend not looking at the diff, but rather look at the new content from scratch.

Thanks!

@cydanil cydanil marked this pull request as draft May 16, 2020 10:41
@cydanil
Copy link
Collaborator Author

cydanil commented May 16, 2020

I'm torn between merging the one test that passes, and adding the others as they get fixed, and merging once all are fixed.

It's certainly going to take a while, as a lot of the interface has changed since the tests were written.
For instance, there's no more Redo action.

@maweki
Copy link
Collaborator

maweki commented May 16, 2020

Since broken tests are no use to anyone, I am fine with merging early. Might also give other people an opening to fix smaller stuff.



with TemporaryDirectory(prefix='gourmet_', suffix='_test_reccard') as tmpdir:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be in an if __name__ == '__main__': block or something? Putting it like this means the module will run its own tests on import, and then the test runner (nose) will run them again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I get it - the test functions need a reccard instance. I think this block should still be in a test function or method, itself, so that it's not trying to run the tests when it's imported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Certainly! But first I'd like the whole shebang to work as intended. However, the structure of the application has changed quite a bit since these tests were implemented, as you noted earlier.

And so I'm currently attempting to make it work, and then I will restructure the tests :)

I think they needn't be interdependent, and each could have their own reccard instance.

Thanks for checking though, I'd have missed that point!

Tests rely on one another, to avoid running them at import, they are wrapped by a single function call
@takluyver
Copy link
Collaborator

This appears to be passing on CI now :-)

@cydanil
Copy link
Collaborator Author

cydanil commented Jun 25, 2020

After over two months of work leading to it, I'm not sure I can let it go ever again!... :p

I wanted to refactor it more: a good thing to do would be to decouple the tests from one another. But it's becoming too time consuming, when I could focus on making the application more usable.

Therefore, I'll rebase and update once #132 is in, and then leave it so. It isn't perfect, but at least it will provide insights in case of regressions.

@cydanil cydanil marked this pull request as ready for review June 26, 2020 18:18
@cydanil
Copy link
Collaborator Author

cydanil commented Jun 26, 2020

That's it.

@cydanil
Copy link
Collaborator Author

cydanil commented Jul 2, 2020

One test has been disabled, as it's becoming too time consuming to keep working on these tests. That test, which tests the sensitivity of the save push button upon undoing, is better suited to be reworked with dogtail.

I still left the code, such that in the future, when someone asks why it's disabled, we can task them with fixing it.

Other than that, the three other tests work and have been verified to be correct!
If there are no further comments, I will merge over the weekend :)

@cydanil cydanil merged commit f79e8e3 into kirienko:master Jul 3, 2020
@cydanil cydanil deleted the test/reccard branch July 3, 2020 23:44
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.

Fix test_reccard.py
3 participants