-
Notifications
You must be signed in to change notification settings - Fork 2
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
remove outdated tests and mark the ones we need to skip for now #516
Conversation
} | ||
event.build(**safety_meeting_info) | ||
|
||
current_meetings = LAMetroEvent.current_meeting() | ||
|
||
# Assert we did not return any current meetings. | ||
assert not current_meetings | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have this attribute on the Events model anymore. It was a complicate bit of code, that we've handled upstream by making sure minutes are attache to the right meeting in the scraper code.
@@ -51,58 +54,28 @@ def test_agenda_pdf_form_error(): | |||
assert agenda_pdf_form.is_valid() == False | |||
|
|||
|
|||
def test_updates_made_true(event, event_document): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't keep this exact behavior and instead will do what we describe her: #509
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @fgregg! This is a bear of a task. Left a few comments inline.
tests/docker-compose.yml
Outdated
@@ -1,6 +1,19 @@ | |||
version: '2.4' | |||
|
|||
services: | |||
app: | |||
test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why define a new service instead of overriding app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it didn't occur to me to override. they seem quite different.
I would say that the intent of docker-compose -f docker-compose.yml -f tests/docker-compose.yml run test
seems a lot clearer to me than docker-compose -f docker-compose.yml -f tests/docker-compose.yml run app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's our current practice to override the application service to run the tests because it reduces repeat configuration code and attempts to ensure as much consistency between app and test environments as possible. (The latter will be more compelling when we start to use our containers in deployed environments, IMO, but it's there nonetheless.) I'd prefer to stick with our pattern and hear proposals for changes over in how-to
, if that's all right with you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Are you able to run the tests locally? I'm getting this exception:
|
yes, I can run locally. |
Got it, somehow had an old test database lurking around. Dropped it manually, resolved the problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks, @fgregg! LGTM.
Thanks for the review. |
Overview
This PR updates the tests. It adjust some tests, removes some unnecessary ones, and skips a few that will fail until we bring in other PRs.
Testing Instructions
docker-compose -f docker-compose.yml -f tests/docker-compose.yml run test