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

anettleship commented 1 year ago

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.

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

anettleship commented 1 year ago

There's a failing check in CircleCI regarding Snyk and a missing api code - I don't believe this happened when I sent my draft commit 5 days ago. Does anyone know if this is already captured as an outstanding issue?

anettleship commented 1 year ago

This is the Snyk error I'm seeing, by clicking through the details button above. I don't know if I get this fully, but I'm assuming that Snyk is performing security checks within CircleCI, but is missing the Snyk auth token in environment variables.

What I don't understand currently is how environment variables are set in circle CI - I'm assuming we have some sort of secrets manager to provide this. I'm going to message on the dev channel for assistance on this one.

SamWinterhalder commented 1 year ago

@giulio-giunta Are you aware of this?

giulio-giunta commented 1 year ago

@anettleship The SNYK token is configured as an environment variable in the 'web' project settings, so that's where Circleci pulls it from.

I'll take a look at this issue later in the afternoon.

Screenshot_2023-05-16_09-20-02

giulio-giunta commented 1 year ago

Hi @anettleship, The issue is that you tried to merge from your forked repository, which we do not allow in the Circleci settings because it may lead to secrets leaking. In fact, the branch you want to merge from is "anettleship:doc-633-tests_not_passing_locally".

In order to commit your work, you'll need to clone the "web" project and create a new branch and PR.

anettleship commented 1 year ago

Oh shoot, my mistake, I'll look into that and update the PR, thanks for looking into it!

giulio-giunta commented 1 year ago

You're welcome

On Tue, 16 May 2023, 22:38 anettleship, @.***> wrote:

Oh shoot, my mistake, I'll look into that and update the PR, thanks for looking into it!

— Reply to this email directly, view it on GitHub https://github.com/GeekZoneHQ/web/pull/664#issuecomment-1550387938, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANPJIC6OG472O5SC77PCZUDXGPXW7ANCNFSM6AAAAAAX5BUPUY . You are receiving this because you were mentioned.Message ID: @.***>

anettleship commented 1 year ago

Closing this Pull request in favour of this: https://github.com/GeekZoneHQ/web/pull/667

GeekZoneHQ / web