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

Accommodate User Accounts functional tests #1788

Merged
merged 4 commits into from
Sep 25, 2017
Merged

Conversation

seav
Copy link
Contributor

@seav seav commented Sep 12, 2017

Note: This PR is branched off from PR #1774. To see the specific changes for this PR, do a branch comparison: functest/loadfixtures...functest/accounts

Proposed changes in this pull request

This PR incorporates changes to accommodate automated User Accounts tests in the cadasta-test repo.

  • Update the tag of cadasta-test repo to v0.2.1.
  • Update settings for Travis to use a dummy email backend and set the max file upload size (for the future).
  • Update the runtests-functional bash script to run non-file-upload tests using BrowserStack and the remaining tests using the local ChromeDriver.
  • Update the runtests.py Python script to pass on extra command-line arguments to pytest when running under --functional.

When should this PR be merged

The following conditions need to be met:

Risks

No risks foreseen.

Follow-up actions

Continue functional test coding.

Checklist (for reviewing)

General

  • Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
    • Review 1
    • Review 2
  • Is the PR labeled correctly? It should have the migration label if a new migration is added.
    • Review 1
    • Review 2
  • Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.
    • Review 1
    • Review 2

Functionality

  • Are all requirements met? Compare implemented functionality with the requirements specification.
    • Review 1
    • Review 2
  • Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.
    • Review 1
    • Review 2

Code

  • Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
    • Review 1
    • Review 2
  • Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
    • Review 1
    • Review 2
  • Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.
    • Review 1
    • Review 2
  • Is the code documented sufficiently? Large and complex classes, functions or methods must be annotated with comments following our code-style guidelines.
    • Review 1
    • Review 2
  • Has the scalability of this change been evaluated?
    • Review 1
    • Review 2
  • Is there a maintenance plan in place?
    • Review 1
    • Review 2

Tests

  • Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
    • Review 1
    • Review 2
  • If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
    • Review 1
    • Review 2
  • If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?
    • Review 1
    • Review 2

Security

  • Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
    • Review 1
    • Review 2
  • Are all UI and API inputs run through forms or serializers?
    • Review 1
    • Review 2
  • Are all external inputs validated and sanitized appropriately?
    • Review 1
    • Review 2
  • Does all branching logic have a default case?
    • Review 1
    • Review 2
  • Does this solution handle outliers and edge cases gracefully?
    • Review 1
    • Review 2
  • Are all external communications secured and restricted to SSL?
    • Review 1
    • Review 2

Documentation

  • Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
    • Review 1
    • Review 2
  • Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
    • Review 1
    • Review 2
  • Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.
    • Review 1
    • Review 2

@seav seav changed the title Accomodate User Accounts functional tests Accommodate User Accounts functional tests Sep 12, 2017
@seav seav force-pushed the functest/accounts branch 2 times, most recently from ccc6c48 to 5ddbdf9 Compare September 18, 2017 18:08
@seav
Copy link
Contributor Author

seav commented Sep 19, 2017

Note: Test PR using the accounts branch of the cadasta-test repo has passed the Travis build.

@seav seav force-pushed the functest/accounts branch from 5ddbdf9 to e519bca Compare September 20, 2017 03:49
@seav seav force-pushed the functest/accounts branch from 681059f to 8687aa3 Compare September 20, 2017 17:49
@seav seav requested a review from alukach September 21, 2017 07:17
@seav seav force-pushed the functest/accounts branch from 8687aa3 to f8dcb19 Compare September 21, 2017 07:33
@seav seav force-pushed the functest/accounts branch 2 times, most recently from 69bf24e to 8bf62aa Compare September 23, 2017 05:01
@seav seav force-pushed the functest/accounts branch from 8bf62aa to 9ec877f Compare September 23, 2017 07:13
@seav
Copy link
Contributor Author

seav commented Sep 25, 2017

I added a commit a couple of days ago. Builds were encountering errors (the test that uses the local ChromeDriver, not BrowserStack) as well as when running tests in the VM. This error started occurring when Chromium was updated to version 61 late last week. Other people have also encountered the same error. Please refer to this Ubuntu bug report.

For the meantime, and in order not to hold up this PR, I have switched to using Firefox+GeckoDriver. For the Travis build, this should only affect just 1 functional test (covering 4 test cases). The rest of the tests still succeed using BrowserStack.

Copy link
Contributor

@alukach alukach left a comment

Choose a reason for hiding this comment

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

Required a complete rebuild of my dev VM but this worked. Nice job!

As an aside, I do wonder if we could speed up these tests by altering the test class so that it uses the same browser rather than closing and re-opening it after every test.

@amplifi amplifi merged commit ff3ad42 into master Sep 25, 2017
@amplifi amplifi deleted the functest/accounts branch September 25, 2017 17:30
@seav
Copy link
Contributor Author

seav commented Sep 26, 2017

As an aside, I do wonder if we could speed up these tests by altering the test class so that it uses the same browser rather than closing and re-opening it after every test.

I also thought of this possible improvement. This does need some small modifications to the test teardown such as logging out any logged in user. Also, I still need to see if there are any time limits to a single BrowserStack session which would become long if we don't shut down the remote browser driver; I haven't found any documentation regarding this yet. But anyway, a good compromise would be to start and stop the browser session on a module basis instead of the current class method basis.

@oliverroick
Copy link
Member

@seav That's definitely something worth looking into. Running the functional that we have so far already takes ~35 min, so anything that makes it faster is more than welcome.

@seav
Copy link
Contributor Author

seav commented Sep 26, 2017

@oliverroick, @alukach: I did a couple of experiments on how to make functional tests run faster and here are the results:

Based on the results above, maybe we should only reserve using BrowserStack for commits to the master branch itself and use the local webdriver otherwise.

@alukach
Copy link
Contributor

alukach commented Sep 26, 2017

@seav Can you remind me what BrowserStack is offering? Just a richer set of browsers to test against?

@seav
Copy link
Contributor Author

seav commented Sep 27, 2017

@alukach, yes. They also include a lot of mobile browsers for Android and iOS, something that we would want to eventually leverage as well. I also like that apart from automated tests, you yourself can use any desktop or mobile browser to view any website via BrowserStack.

@seav seav mentioned this pull request Nov 22, 2017
46 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants