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

Adjust instructions for running EF integration tests #457

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

addisonbeck
Copy link

@addisonbeck addisonbeck commented Oct 17, 2024

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-10863

📔 Objective

This commit adds up to date documentation for running entity framework integration tests for server. It has a companion PR for adding some of the mentioned functionality in bitwarden/server#4912

📸 Screenshots

Screenshot 2024-10-17 at 7 54 35 PM

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@addisonbeck addisonbeck requested a review from a team as a code owner October 17, 2024 23:48
Copy link

github-actions bot commented Oct 17, 2024

Logo
Checkmarx One – Scan Summary & Details005930db-f5aa-486f-aab1-377b58d19c83

No New Or Fixed Issues Found

@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863-ef-integration-tests branch from 224b67a to 8b5527f Compare October 17, 2024 23:52
Copy link

cloudflare-workers-and-pages bot commented Oct 17, 2024

Deploying contributing-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 05d0165
Status: ✅  Deploy successful!
Preview URL: https://6e13bff8.contributing-docs.pages.dev
Branch Preview URL: https://ac-addison-pm-10863-ef-integ.contributing-docs.pages.dev

View logs

@addisonbeck addisonbeck force-pushed the ac/addison/pm-10863-ef-integration-tests branch from 8b5527f to 05d0165 Compare October 17, 2024 23:54
@withinfocus withinfocus requested a review from a team October 18, 2024 13:26
Copy link

@rkac-bw rkac-bw left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +227 to +238
"databases:0:type": "Postgres",
"databases:0:connectionString": "Host=localhost;Username=postgres;Password=_________;Database=ef_test",
"databases:0:enabled": "true",
"databases:1:type": "Sqlite",
"databases:1:enabled": "true",
"databases:1:connectionString": "Data Source=_________",
"databases:2:type": "MySql",
"databases:2:connectionString": "server=localhost;uid=root;pwd=_________;database=ef_test",
"databases:2:enabled": "true",
"databases:3:type": "SqlServer",
"databases:3:connectionString": "Server=localhost;Database=ef_test;User Id=SA;Password=_________;Encrypt=True;TrustServerCertificate=True;",
"databases:3:enabled": "true"
Copy link
Member

Choose a reason for hiding this comment

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

I suggested changing the format of these user secrets. If you do that, this will need to be updated. If you don't want to do that, then we should at least note here (maybe in a callout) that the index of each type must be maintained for the migrate script to work.

Comment on lines +201 to +220
- This block is used for your primary Bitwarden database for each supported provider type. These are
what your running development server will connect to. You should already have passwords set up in
this block.
- If you do: ensure the username/password combos for each of your global database types are
applied to their siblings in the `databases` block.
- If you do not: please see the user secrets section of the server setup guide for doing this.
```
"sqlServer": {
"connectionString": "Server=localhost;Database=vault_dev;User Id=SA;Password=___________;Encrypt=True;TrustServerCertificate=True;"
},
"postgreSql": {
"connectionString": "Host=localhost;Username=postgres;Password=_________;Database=vault_dev"
},
"mySql": {
"connectionString": "server=localhost;uid=root;pwd=_________;database=vault_dev"
},
"sqlite": {
"connectionString": "Data Source=/path/to/bitwardenServer/repository/server/dev/db/bitwarden.sqlite"
},
```
Copy link
Member

@eliykat eliykat Oct 23, 2024

Choose a reason for hiding this comment

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

I don't think this is required. It's already covered in the sections above so this is largely repeating information. I think you could just start with the following paragraph and it would work just as well or better.


You can then run just those tests from the `test/Infrastructure.IntegrationTest` folder using
`dotnet test`.
In your `server/dev/secrets.json` file find these two blocks of secrets:
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Theserver/ prefix might be confusing for some. This name is not guaranteed and we never reference it anywhere. It's almost always secrets.json.

```

With connection strings applied to your projects: ensure your databases are all migrated using
`pwsh server/dev/migrate.ps1 --all`. Then you can run EF tests from the
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Theserver/ prefix might be confusing for some and it's not guaranteed.

Comment on lines -224 to -225
You can then run just those tests from the `test/Infrastructure.IntegrationTest` folder using
`dotnet test`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this text more.

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.

4 participants