Closes #633 Tests not passing locally but are in CI - updated_v2

anettleship commented 1 year ago

Replaces: https://github.com/GeekZoneHQ/web/pull/664 https://github.com/GeekZoneHQ/web/pull/667

This pull request pulls from a new branch added as GeekZoneHQ/web:doc-633-tests_not_passing_locally_updated, instead of my own forked repository used in the previous pull request: anettleship/GeekZone-web-adrian/tree/doc-633-tests_not_passing_locally, and instead of a branch pushed back to GeekZoneHQ/web:doc-633-tests_not_passing_locally from my forked repository, but setting GeekZoneHQ/web as a second linked repository. In the previous PRs we ran into a CircleCI issue because I had issued the pull request from files in my my own forked repository in my github account rather than a branch cloned from and pushed back to GeekZoneHQ/web.

--

Problem: When running python3 manage.py test locally in WSL, it runs all 33 tests but FAILED (failures=1, errors=6) The same tests are all passing in CircleCI.

Solution Amend project readme.md file, where dev set up is documented, to instruct user to create test user environment variables for TEST_USER_PASSWORD and TEST_USER_PASSWORD_BAD in web/.env to align with values currently present in web/.env.dev.

The readme directs the user to create a file at web/.env with the following commands listed below. These do not currently create the two environment variables and values:

TEST_USER_PASSWORD=k38m1KIhIUzeA^UL TEST_USER_PASSWORD_BAD=password

When tests run locally, these environment variables are null. This caused the errors reported in the bug report (failures=1, errors=6). For the failing test: test_signed_in_users_cannot_register, the test failed as it was unable to login successfully with a null password, so after failing to log in, the test was able to register a new user, so it returned http 200 for success rather than the 302 redirect.

I have amended the two commands provided in the readme that create the .env file for unix and windows users from and to...

Windows Before:

echo "DEBUG=1 DATABASE_USER=postgres DATABASE_NAME=postgres DATABASE_HOST=localhost DATABASE_PASSWORD=postgres DATABASE_PORT=5432" | tee web/.env

Windows After:

echo "DEBUG=1 DATABASE_USER=postgres DATABASE_NAME=postgres DATABASE_HOST=localhost DATABASE_PASSWORD=postgres DATABASE_PORT=5432 TEST_USER_PASSWORD=k38m1KIhIUzeA^UL TEST_USER_PASSWORD_BAD=password" | tee web/.env

Unix Before:

cat < web/.env DEBUG=1 DATABASE_USER=postgres DATABASE_NAME=postgres DATABASE_HOST=localhost DATABASE_PASSWORD=postgres DATABASE_PORT=5432 EOF

Unix After:

cat < web/.env DEBUG=1 DATABASE_USER=postgres DATABASE_NAME=postgres DATABASE_HOST=localhost DATABASE_PASSWORD=postgres DATABASE_PORT=5432 TEST_USER_PASSWORD=k38m1KIhIUzeA^UL TEST_USER_PASSWORD_BAD=password EOF

Motivation and Context

Tests were failing, now they pass.

How Has This Been Tested?

I've tested this fully on Linux to determine that the tests fail in the expected places following the readme and using the first command, but when the .env file is created with the amended command instead - i.e. the two new environment variable are present in web/.env, all tests pass.

Screenshot from 2023-05-15 17-46-13

On Windows I have tested the powershell command to ensure that the .env file is created correctly with the additional lines, but I have not set up the project and run the tests.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue) - change to documentation only
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[x] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes - no new code.
[x] All new and existing tests passed.

anettleship commented 1 year ago

We have two blockers to this PR passing successfully:

1). this branch changes documentation only so is named doc-, however, our Circle CI config ignores doc- named branches by default, resulting in no result being returned from Circle CI checks, where a positive result is required.

2). there is an issue with Snyk workflow, to be investigated, leading to a failure in these checks.

anettleship commented 1 year ago

Snyk security checks are failing with the following message:

`Tested 46 dependencies for known issues, found 4 issues, 12 vulnerable paths.

Issues to fix by upgrading dependencies:

Upgrade pillow@9.3.0 to pillow@9.4.0 to fix ✗ Denial of Service (DoS) (new) [Medium Severity][https://security.snyk.io/vuln/SNYK-PYTHON-PILLOW-5[48](https://github.com/GeekZoneHQ/web/actions/runs/5025112864/jobs/9011664862?pr=669#step:4:49)9784] in pillow@9.3.0 introduced by pillow@9.3.0

Upgrade requests@2.25.1 to requests@2.31.0 to fix ✗ Information Exposure (new) [Medium Severity][https://security.snyk.io/vuln/SNYK-PYTHON-REQUESTS-5595[53](https://github.com/GeekZoneHQ/web/actions/runs/5025112864/jobs/9011664862?pr=669#step:4:54)2] in requests@2.25.1 introduced by requests@2.25.1 and 2 other path(s)

Upgrade sqlparse@0.4.2 to sqlparse@0.4.4 to fix ✗ Regular Expression Denial of Service (ReDoS) [High Severity][https://security.snyk.io/vuln/SNYK-PYTHON-SQLPARSE-[54](https://github.com/GeekZoneHQ/web/actions/runs/5025112864/jobs/9011664862?pr=669#step:4:55)26157] in sqlparse@0.4.2 introduced by sqlparse@0.4.2 and 5 other path(s)

Upgrade tornado@6.1 to tornado@6.3.2 to fix ✗ Open Redirect (new) [Medium Severity][https://security.snyk.io/vuln/SNYK-PYTHON-TORNADO-[55](https://github.com/GeekZoneHQ/web/actions/runs/5025112864/jobs/9011664862?pr=669#step:4:56)37286] in tornado@6.1 introduced by tornado@6.1 and 1 other path(s)`

anettleship commented 1 year ago

I've updated the stated dependencies in the project requirements, Snyk tests are now passing. All software tests are passing locally.

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

giulio-giunta commented 1 year ago

Hi @anettleship, The "main-web" check related to the Circleci pipeline is required as well, but it fails the deploy jobs because there is an issue with Terraform building the infrastructure where the application is deployed. I raised a ticket with Terraform Cloud because haven't been able to figure out the cause, so in the meantime, I think we can momentarily comment out the deploy jobs in the Circleci pipeline config to pass the check and build the image.

Regarding the first issue you mentioned, we need to remove the line that prevents the cicd pipeline to run when the triggering branch is named "doc", however, we need to add a step at the beginning of the "build-test-publish" job that automatically returns success if the branch is named "doc" and prevents the pipeline from running if the change only involves the documentation.

anettleship commented 1 year ago

I've just started a new job role, so I've de-assigned the original issue, rather than leave it hanging.

Tasks outstanding are to log issues or directly carry out the above actions described by @giulio-giunta

Once the pipe line checks are green and the doc updates are published, this issue should be resolved.

I'd like to do this myself, but I hope I've done enough to make this an easy win for someone else if they get there first!

GeekZoneHQ / web