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

Added basic user back in to script for Cypress testing #442

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

droberts-ctrlo
Copy link
Contributor

No description provided.

@tomhukins
Copy link
Member

I realise that getting your development setup working is important and GADS has various different database setup scripts that are a bit of a mess, including setup_database and seed_database.pl that it calls.

The Cypress tests currently pass in GitHub Actions. I believe these commits attempt to get them working in your development environment. Single line commit messages easy to write, and commonly used, but there's a lot of value in writing multiple line commit messages that explain your thought process, including things you tried that didn't work out, and things you considered but decided against.

I notice we use the environment variable TEST in both setup_database and seed_database.pl and git grep TEST shows we use it in GitHub Actions for testing. However, I believe the commits in this pull request also use if for running code and tests in containers. A more detailed commit message could explain this. Although I like detailed explanatory commit messages, I like self-describing variable names even more. Something more descriptive than TEST would make the code easier to understand.

Instead of scattering code that depends on a given environment variable throughout two different scripts, it would be good to see related code consolidated in one place. It would also be good to avoid hardcoded usernames and passwords, instead deriving them from environment variables or command line flags.

@abeverley
Copy link
Contributor

Thanks Tom, useful comments, we will discuss.

@abeverley abeverley merged commit 5d08f0e into ctrlo:dev Jul 25, 2024
6 checks passed
@droberts-ctrlo droberts-ctrlo deleted the cypress-fix branch August 6, 2024 12:59
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.

3 participants