Skip to content

Improve automated tests #359

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
Closed
wants to merge 8 commits into from
Closed

Improve automated tests #359

wants to merge 8 commits into from

Conversation

@hanneskaeufler
Copy link
Contributor

@hanneskaeufler hanneskaeufler commented Dec 9, 2019

Reference to a related issue in the repository

None - These are out of context quality of life improvements in the automated test suite

Add a description

I saw some long-hanging fruit in improving these tests. I hope the commit messages do
well enough to explain the change/intention behind it. If not, feel free to comment on details :)

Check the checklist

  • My code and comments follow the style guidelines and contributors guidelines of this project.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests / travis ci pass locally with my changes.

@hanneskaeufler
Improve error message
22503c9 
When forgetting to install the protobuf compiler,
an error message is shown which had a missing
space between two words.
@hanneskaeufler
Remove trailing whitespace
e9c1e12
@jdsika jdsika requested a review from vkresch December 9, 2019 16:33
@jdsika jdsika added the Quality Quality improvements. label Dec 9, 2019
@jdsika jdsika added this to the v3.1.3 milestone Dec 9, 2019
@jdsika
Copy link
Contributor

jdsika commented Dec 9, 2019

Thank you Hannes for your contribution. It is very welcome! @vkresch will review it tomorrow

vkresch
vkresch requested changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

@vkresch vkresch 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 the improvement! Looks nice and pythonic :). Just change one line for the newline warning and we can merge it.

vkresch
vkresch approved these changes Dec 11, 2019
View reviewed changes
Copy link
Contributor

@vkresch vkresch left a comment

Choose a reason for hiding this comment

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

Thanks!

@vkresch
Copy link
Contributor

vkresch commented Dec 12, 2019 •
edited
Loading

@jdsika there seems to be an issue with the status report form travis to merge this PR see here for more how to maybe solve it https://travis-ci.community/t/known-issue-travis-ci-reports-expected-waiting-for-status-to-be-reported-on-the-github-status-api-but-the-status-never-arrives/1154

@jdsika
Copy link
Contributor

jdsika commented Dec 12, 2019

I do not find the option. Very strange...

@jdsika
Copy link
Contributor

jdsika commented Dec 13, 2019

@hanneskaeufler I invited you to the organization. Maybe this is the problem of the status not being properly reported. Can you accept the invite and make an empty commit (not reopen of PR) as a member? If this does not work @vkresch will copy your stuff and make a branch.

@hanneskaeufler
Read file in one go
60b9a72 
There is no reason to traverse the file
line by line if all we care about is the
last character.

This also uses unittests subTest to show
the file which fails the test.
@hanneskaeufler
Fix header comment
6950230
@hanneskaeufler
Use subTest to show all failing files
8a51a49 
When one file is failing, due to the
fact that all tests contain for loops,
the other failures are swallowed because python
stops running the test on the first assertion.
With subTest one can introduce a subcontext which
allows showing all of the failed files as well a
diagnostic as to which file failed.
@hanneskaeufler
Use enumeration index for line count
0a6c162 
Python provides mechanisms for that, no
reason to manually track a count.
@hanneskaeufler
Remove unused imports, only glob once
96698ca 
The globs where repeated a whole bunch of times
which is both information duplication as well
as unneccessary work.
@hanneskaeufler
Remove needless conditionals in front of assert
5c1a1bf 
There is no reason to guard the assertions
in the conditional.
@hanneskaeufler
Copy link
Contributor Author

hanneskaeufler commented Dec 13, 2019

@hanneskaeufler I invited you to the organization. Maybe this is the problem of the status not being properly reported. Can you accept the invite and make an empty commit (not reopen of PR) as a member? If this does not work @vkresch will copy your stuff and make a branch.

Thanks! I accepted and rebased one more time ...

@hanneskaeufler hanneskaeufler mentioned this pull request Dec 13, 2019
Merged
6 tasks
@jdsika jdsika removed this from the v3.2.0 milestone Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdsika jdsika jdsika approved these changes

@vkresch vkresch vkresch approved these changes

Assignees

No one assigned

Labels

Quality Quality improvements.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hanneskaeufler @jdsika @vkresch