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

Mccalluc/cypress continued #2239

Merged
merged 65 commits into from Oct 20, 2017
Merged

Mccalluc/cypress continued #2239

merged 65 commits into from Oct 20, 2017

Conversation

mccalluc
Copy link
Member

@mccalluc mccalluc commented Oct 16, 2017

Here's a first look at what testing with cypress would look like.

Plus:

  • The test runner UI for local development
  • Their dashboard service, so we can see the results of runs on Travis
  • Tests are very easy to write, and run behavior is predictable.

Minus:

  • Vendor lock-in
  • This will be easiest to use for writing end-to-end integration tests, where we install solr on Travis; Alternatively, API calls can be mocked, but that would make it much harder to write each test.
  • It is impossible to mock any of the internals in python: Either we hit an actual server, or we don't.
  • Pollution of development environment: Since real objects are created by the tests, they would need to be explicitly cleaned up, as well, if we care about that.

I think this could be useful for us, but I think having more eyes on it before going forward would be good.


PS: As of today it's open source.

@mccalluc
Copy link
Member Author

Still need to revert the Selenium deletion, but after that I think it's ready to merge.

@@ -6,6 +6,4 @@
TEMPLATE_DEBUG = DEBUG

# Required when DEBUG = False
ALLOWED_HOSTS = get_setting("ALLOWED_HOSTS")

EMAIL_BACKEND = "django.core.mail.backends.smtp.EmailBackend"
Copy link
Member Author

Choose a reason for hiding this comment

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

@scottx611x : Looking at this again, I remember that we did need to add console.EmailBackend for testing, but in production I'd think we'd still want this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, both are needed in production.

Copy link
Member

Choose a reason for hiding this comment

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

@hackdna Doesn't the EMAIL_BACKEND in aws.py override this anyways?

We were wondering if this was even needed in prod.py if theres no accompanying SMTP server running by default.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. The purpose of a prod config is to run the site with real users which means you need a real SMTP backend to send email.

Copy link
Member Author

Choose a reason for hiding this comment

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

Talking with Scott, realized that we were probably too fixated on running these tests in production mode. If we we ran it in development mode, the only real difference is that if something did go wrong we'd get the extra debug information, and that's a good thing.

@@ -55,5 +55,4 @@
include refinery::neo4j
include refinery::postgresql
include refinery::python
include refinery::selenium
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be brought back if we're going to keep selenium around and ween ourselves off of it.

@@ -16,6 +16,7 @@ class Meta:
name = "Test DataSet - {}".format(uuid)
creation_date = datetime.now()
modification_date = datetime.now()
slug = None
Copy link
Member

Choose a reason for hiding this comment

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

I think you could already use the factory here to create a Dataset with a slug without adding this line.
I could also very well be wrong and would like to know if I am!

.travis.yml Outdated
@@ -40,7 +44,9 @@ env:
global:
# These env vars are available to every build
- PYTHONPATH=$PYTHONPATH:../refinery:../refinery/config
- DJANGO_SETTINGS_MODULE=config.settings.prod
- DJANGO_SETTINGS_MODULE=config.settings.dev
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of something more along the lines of on line 76:
python manage.py runserver --insecure --settings=config.settings.dev
so that the cypress testing is the one off thing and we can still run our units against prod settings

Copy link
Member

Choose a reason for hiding this comment

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

FYI, unit tests should be unaffected by Django settings because they should only test small pieces of code in isolation.
Also, --insecure is only needed when DEBUG is False.

Copy link
Member Author

@mccalluc mccalluc Oct 19, 2017

Choose a reason for hiding this comment

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

core runserver only has --noreload, --nothreading, --ipv6 and the option to pick a port.

staticfiles runserver adds --insecure.

But I could just reset that envar before runserver?

( DJANGO_SETTINGS_MODULE=config.settings.dev && python manage.py runserver --insecure & )

... but maybe a better option is to make a new settings file that is like prod in all things, except for email?

Copy link
Member

Choose a reason for hiding this comment

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

There is only one Django dev server. It is invoked by the runserver subcommand. If DEBUG is set to True in the settings (e.g., in dev.py) you simply don't need to use --insecure (that mode is insecure by default).
Btw, the docs you've linked to are for Django 1.11 while we are running 1.7 - there often big differences between versions.

@mccalluc
Copy link
Member Author

mccalluc commented Oct 19, 2017

@hackdna: Thanks for the feedback! I think your concerns have been addressed.

Talked with Jennifer, and she would defer to Scott and me, if we thought this looked reasonable, and I think my last commit manages settings in the way Scott suggested. Unless there are other problems, or tests fail, I'll merge tomorrow morning.

.travis.yml Outdated
- python manage.py add_users_to_public_group
- pushd ui && grunt make && popd
- python manage.py collectstatic --noinput -v 0
- python manage.py runserver --insecure --settings config.settings.prod_except_email &
Copy link
Member

Choose a reason for hiding this comment

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

This configuration doesn't test any actual scenario. In dev mode Django devserver is used with DEBUG=True. In prod mode Apache is used with DEBUG=False.

Also, there are very few bugs that could appear in prod mode but not in dev and they would be highly dependent on the deployment environment anyway, so I would simply run Cypress tests with dev settings.

…from settings.base), but continue to use prod for rest of tests (as has been the case since #1534).
@mccalluc
Copy link
Member Author

Thanks: That makes sense, @hackdna. In the last commit:

  • The python tests will continue to run with prod settings, as has been the case for a while, but runserver will use dev.
  • In base.py we set console email as the default, and that is overridden if running prod.

@mccalluc mccalluc mentioned this pull request Oct 20, 2017
@mccalluc mccalluc merged commit d2f3846 into develop Oct 20, 2017
@mccalluc mccalluc deleted the mccalluc/cypress-continued branch October 20, 2017 14:11
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.

None yet

4 participants