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.

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

CI job for running Python unit tests #10819

Merged
merged 2 commits into from
Jan 22, 2021
Merged

CI job for running Python unit tests #10819

merged 2 commits into from
Jan 22, 2021

Conversation

cameel
Copy link
Member

@cameel cameel commented Jan 19, 2021 •
edited
Loading

NOTE: This PR is based on #10822. Please don't merge until that one is merged.

This job runs the new unit tests introduced in #10675.

I'm running it also on Windows because I want to add some tests for #10676 that ensure that newlines are processed in the same way on CRLF platforms too.

EDIT: Turns out that the tests were not passing on Windows so I have rebased this PR on #10822 which fixes those issues.

@cameel cameel self-assigned this Jan 19, 2021
@cameel cameel force-pushed the python-unit-tests-in-ci branch 13 times, most recently from 5511aa0 to 9de4bf2 Compare January 19, 2021 16:54
@cameel cameel mentioned this pull request Jan 19, 2021
Merged
@cameel cameel marked this pull request as ready for review January 19, 2021 17:11
@cameel cameel marked this pull request as draft January 20, 2021 11:20
@cameel cameel force-pushed the python-unit-tests-in-ci branch 5 times, most recently from 657f989 to 006a604 Compare January 20, 2021 16:26
@cameel
Copy link
Member Author

cameel commented Jan 20, 2021

Looks like bytecode comparison passed. I just pushed small tweaks now but they should not really change the result. This is now ready for review.

cameel
cameel commented Jan 20, 2021
View reviewed changes
.circleci/config.yml Outdated
# NOTE: For bytecode generation we need the input files to be byte-for-byte identical on all
# platforms so line ending conversions must absolutely be disabled.
- run: git config --global core.autocrlf false
Copy link
Member Author

Choose a reason for hiding this comment

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

We could put this in .gitattributes but then it would also change for the build job and for contributors who work on Windows. I think that for the build job either is fine (with it enabled we're getting extra testing for CRLF versions) but not for users - we'd probably start getting CRLF endings in PRs. So I'm changing this only for specific jobs.

@cameel cameel marked this pull request as ready for review January 20, 2021 18:04
@cameel cameel force-pushed the python-unit-tests-in-ci branch from 006a604 to def25e1 Compare January 21, 2021 10:31
@cameel
Copy link
Member Author

cameel commented Jan 21, 2021 •
edited
Loading

The command to run the Python tests locally on your machine (PYTHONPATH="scripts/:$PYTHONPATH" python -m unittest discover --start-directory test/scripts/) is pretty cumbersome so I added a wrapper script that does it. Now it can be executed from any directory and as a bonus the command is now identical on Windows (which was a bit tricky to get right).

I'm moving the PR back to draft for a moment because now that I pushed it, I need to resolve the CI failures.

@cameel cameel marked this pull request as draft January 21, 2021 10:34
@cameel cameel force-pushed the python-unit-tests-in-ci branch 2 times, most recently from aeda9a5 to e9ef298 Compare January 21, 2021 10:39
@cameel cameel force-pushed the python-unit-tests-in-ci branch from 1b135a0 to 4465e6e Compare January 21, 2021 11:46
@cameel cameel mentioned this pull request Jan 21, 2021
Merged
@cameel cameel force-pushed the bytecode-report-windows-compatibility-fixes branch from 326b814 to b8329c0 Compare January 21, 2021 15:24
@cameel cameel force-pushed the python-unit-tests-in-ci branch from 4465e6e to 8c88bb0 Compare January 21, 2021 15:24
@cameel cameel force-pushed the bytecode-report-windows-compatibility-fixes branch from b8329c0 to 83d65ba Compare January 22, 2021 11:18
@cameel cameel force-pushed the python-unit-tests-in-ci branch from 8c88bb0 to 01756b8 Compare January 22, 2021 11:18
hrkrshnn
hrkrshnn previously approved these changes Jan 22, 2021
View reviewed changes
Copy link
Member

@hrkrshnn hrkrshnn left a comment

Choose a reason for hiding this comment

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

Looks good. Again one more review is nice before merging.

Base automatically changed from bytecode-report-windows-compatibility-fixes to develop January 22, 2021 12:10
@cameel cameel dismissed hrkrshnn’s stale review January 22, 2021 12:10

The base branch was changed.

@cameel
Copy link
Member Author

cameel commented Jan 22, 2021 •
edited
Loading

@hrkrshnn I just merged #10822 and looks like it dismissed your review :|

@cameel cameel force-pushed the python-unit-tests-in-ci branch from 01756b8 to 1a6fefd Compare January 22, 2021 12:15
@cameel
Copy link
Member Author

cameel commented Jan 22, 2021

The change I just pushed is just a minor comment fix (will executed -> will be executed).

leonardoalt
leonardoalt reviewed Jan 22, 2021
View reviewed changes
.circleci/config.yml
- checkout
- run:
name: Python unit tests
command: python3 test/pyscriptTests.py
Copy link
Member

Choose a reason for hiding this comment

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

Is python3 for sure installed?

Copy link
Member

Choose a reason for hiding this comment

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

It runs in the CI :) So, yes?

Copy link
Member

Choose a reason for hiding this comment

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

it's ubuntu so, you never know...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. It's actually the plain python executable that is not :) Initially I used python and I was getting errors saying that it does not exist.

On Ubuntu the python symlink is a part of some package other than python (it's in python-pip I think?) so if you just install Python 3, it's not there.

leonardoalt
leonardoalt reviewed Jan 22, 2021
View reviewed changes
test/pyscriptTests.py Outdated Show resolved Hide resolved
hrkrshnn
hrkrshnn previously approved these changes Jan 22, 2021
View reviewed changes
@cameel cameel force-pushed the python-unit-tests-in-ci branch 2 times, most recently from 16aa5c0 to 564f963 Compare January 22, 2021 12:39
leonardoalt
leonardoalt previously approved these changes Jan 22, 2021
View reviewed changes
@leonardoalt
Copy link
Member

python3.exe fails right away though haha

@cameel cameel force-pushed the python-unit-tests-in-ci branch from 564f963 to cf147fa Compare January 22, 2021 12:41
@cameel
Copy link
Member Author

cameel commented Jan 22, 2021

Yeah :)

python3.exe : The term 'python3.exe' is not recognized as the name of a cmdlet, function, script file, or operable 
program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

Changed back.

@cameel cameel merged commit aef724c into develop Jan 22, 2021
@cameel cameel deleted the python-unit-tests-in-ci branch January 22, 2021 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hrkrshnn hrkrshnn hrkrshnn approved these changes

@leonardoalt leonardoalt leonardoalt approved these changes

Assignees

@cameel cameel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cameel @leonardoalt @hrkrshnn