Skip to content

Add a way to easily test units #1386

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

Merged
merged 19 commits into from
Jun 28, 2021

Conversation

mathbunnyru
Copy link
Member

@mathbunnyru mathbunnyru commented Jun 26, 2021

Full description is in the issue.
Fix: #1353

@mathbunnyru
Copy link
Member Author

Works locally!

---------------------------------------------------------------------------------------------- live log call -----------------------------------------------------------------------------------------------
2021-06-26 18:54:03 [    INFO] Searching for units tests in /home/asalikhov/docker-stacks/test/../scipy-notebook/test/units (test_units.py:17)
2021-06-26 18:54:03 [    INFO] Running jupyter/scipy-notebook with args {'detach': True, 'ports': {'8888/tcp': 8888}, 'volumes': {'/home/asalikhov/docker-stacks/test/../scipy-notebook/test/units': {'bind': '/home/jovyan/data', 'mode': 'ro'}}, 'tty': True, 'command': ['start.sh', 'bash', '-c', 'sleep infinity']} ... (conftest.py:80)
2021-06-26 18:54:03 [    INFO] Running unit test: unit_pandas.py (test_units.py:31)
PASSED

@mathbunnyru mathbunnyru requested a review from consideRatio June 26, 2021 15:59
@mathbunnyru
Copy link
Member Author

This seems to be ready.
Overall, simple tests are just a few lines of python code without any pytest / docker, execute and behave exactly the same as before, more readable.
And actually before adding README.md, the number of lines of code reduced, which is also great.

@consideRatio
Copy link
Member

consideRatio commented Jun 26, 2021

@mathbunnyru can you help me help you get this reviewed by applying the autoformatting as a dedicated PR?

For me, the main point of having autoformatters is to avoid ending up with PRs including formatting changes alongside logic changes to help the review process and avoid cluttering git history when git blame is used. I suggest we do pre-commit run --all.

Oh... I tried that and conclude we don't have autoformatting regarding trailing commas or similar. I would suggest we use black as an autoformatter if you want to have it be consistent about such style matters and then we do pre-commit run black --all once in a dedicated PR.

UPDATE: I created #1387

@mathbunnyru
Copy link
Member Author

@mathbunnyru can you help me help you get this reviewed by applying the autoformatting as a dedicated PR?

These were not automatic changes.
I only made changes in test files because I was looking at them and making them consistent.

@mathbunnyru
Copy link
Member Author

@consideRatio you asked to make it easier for you to review these changes.
No problem :)

I made a separate PR, which changes the style in the test files.
#1388
I'm going to merge this as soon as it passes the tests.

Then I'm going to merge master to the branch in this PR and this change will be purely adding unit tests.

In parallel, I'll ask you to merge master in your branch and rerun black.
The reason for this is simple - I've actually given some time to make the code in this repo look good.
And I think manually formatted code + flake8 behaves better than black does.
I will find some examples of such behavior.
Sorry, I think black is great, when the code was not good in the beginning.
But if it already is, then it can do some things, which are not pretty.

@mathbunnyru
Copy link
Member Author

@consideRatio now this is purely feature add.

@mathbunnyru
Copy link
Member Author

@consideRatio now it's your turn to review 😆

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

I've made one suggestion that I think can make this code more robust in case we want to publish to someplace else than dockerhub such as quay.io, but other than that i think this looks amazing! Nice work!

Co-authored-by: Erik Sundell <[email protected]>
@consideRatio
Copy link
Member

LGTM!

@consideRatio consideRatio merged commit 33caee1 into jupyter:master Jun 28, 2021
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.

An ability to easily write modules unit tests
2 participants