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

Use pytest #1143

Merged
merged 1 commit into from
Jun 11, 2021
Merged

Use pytest #1143

merged 1 commit into from
Jun 11, 2021

Conversation

pefoley2
Copy link
Member

@pefoley2 pefoley2 commented May 29, 2021

setup.py test is deprecated, switch to pytest.

@etnguyen03
Copy link
Contributor

etnguyen03 commented May 30, 2021

Apologies if I'm being naive but could we just use and call ./manage.py test?

E: Never mind

@pefoley2
Copy link
Member Author

Sure, but it's IMO a nicer experience.
e.g. you actually get warnings called out.
There's also https://pytest-django.readthedocs.io/en/latest/index.html#why-would-i-use-this-instead-of-django-s-manage-py-test-command

@pefoley2 pefoley2 force-pushed the master branch 3 times, most recently from 1763795 to 19ff2d9 Compare May 30, 2021 02:41
@pefoley2 pefoley2 changed the title Use pytest use pytest May 30, 2021
@pefoley2 pefoley2 force-pushed the master branch 4 times, most recently from cc7c630 to c356692 Compare May 30, 2021 03:31
@pefoley2
Copy link
Member Author

If you look at https://github.com/tjcsl/ion/pull/1143/checks?check_run_id=2703066197, you can see the 21 previously failing tests that weren't being run for whatever reason.

@pefoley2 pefoley2 changed the title use pytest Use pytest May 30, 2021
@pefoley2 pefoley2 marked this pull request as ready for review May 30, 2021 04:15
@pefoley2 pefoley2 requested review from Laur04 and a team as code owners May 30, 2021 04:15
@Laur04 Laur04 mentioned this pull request May 31, 2021
@Laur04
Copy link
Member

Laur04 commented May 31, 2021

Interesting, I can't replicate these failures using pytest or python manage.py test locally using either the current dev or this branch. Do you have an Ion dev env where you are seeing all of these failures locally or is it just in running CI? Both the current method of running tests and pytest both report running 285 tests...I might have a theory, but I'd have to look more. @etnguyen03 can you replicate?

@pefoley2
Copy link
Member Author

I can repo locally by running pytest in my ion venv.
I can't repo w/ ./manage.py test though.
I suspect that manage.py isn't as comprehensive wrt test discovery compared to pytest so it's simply missing some that don't match its expected naming, although I haven't actually confirmed that one way or the other.

@Laur04
Copy link
Member

Laur04 commented May 31, 2021

Both ./manage.py test and pytest are running 285 tests though.

@pefoley2
Copy link
Member Author

Oh...
I see what's going on.
./manage.py test and ./setup.py test match this condition:
https://github.com/tjcsl/ion/blob/master/intranet/settings/__init__.py#L74

Which forces them to use sqlite3, which makes stuff pass that doesn't w/ postgres (like the column length assertions).
adding and False to that if gives:
Traceback (most recent call last):
File "/home/peter/ion/intranet/apps/eighth/tests/test_profile.py", line 155, in test_profile_signup_view
self.assertEqual(scheduled.id, response.context["active_block_current_signup"])
AssertionError: 703 != 706

So it's not so much an issue with ./manage.py test as an issue w/ sqlite.

Given the apparently not great compatibility w/ sqlite, I'd consider switching the ci to use postgres.
Or just using --keepdb, which gives:
real 0m9.249s
for postgres, and:
real 0m9.190s
for sqlite...

@pefoley2 pefoley2 force-pushed the master branch 2 times, most recently from 1cf2e90 to 33d9005 Compare May 31, 2021 21:33
@Laur04
Copy link
Member

Laur04 commented May 31, 2021

you've got some weird branch conflicts going on here...targeting dev should fix that

@pefoley2 pefoley2 changed the base branch from master to dev May 31, 2021 21:37
Copy link
Member

@Laur04 Laur04 left a comment

Choose a reason for hiding this comment

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

This is going to be a fairly extensive PR to fix all of the issues that aren't being caught with the current sqlite database. We also want to be able to run these tests locally, which is going to involve removing the in memory testing altogether since that isn't compatible with postgres. Do you want to (1) leave this PR at just switching CI to pytest and the fixes you made to a few tests and I'll merge it and deal with patching everything else and switching local testing, (2) add onto this, patching everything and switching to postgres for local testing or (3) something else?

@pefoley2 pefoley2 force-pushed the master branch 3 times, most recently from c980682 to 3e5d7f1 Compare June 11, 2021 01:14
@pefoley2 pefoley2 force-pushed the master branch 3 times, most recently from 07c49f5 to 18287f1 Compare June 11, 2021 03:27
@pefoley2
Copy link
Member Author

Yeah, I think I'll just focus this pr on the pytest switch and leave sqlite->postgres to #1144.
I rebased it on latest dev and fixed a bunch more warnings, PTAL

@pefoley2 pefoley2 requested a review from Laur04 June 11, 2021 03:29
@pefoley2 pefoley2 force-pushed the master branch 2 times, most recently from ff6a23f to 589334d Compare June 11, 2021 04:08
setup.py test is deprecated, switch to pytest.

Also fix a bunch of warnings it flagged.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 82.408% when pulling 9b2c390 on pefoley2:master into 711ac75 on tjcsl:dev.

Copy link
Member

@Laur04 Laur04 left a comment

Choose a reason for hiding this comment

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

LGTM

@Laur04 Laur04 merged commit 900edd5 into tjcsl:dev Jun 11, 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.

4 participants