Conversation
Merge main into dev. Please follow the branching strategy to avoid this 😄
Feature/logging
|
Here's the code health analysis summary for commits Analysis Summary
|
WalkthroughThis set of changes introduces end-to-end Playwright tests for the Sinatra application, including new test scripts for core user flows such as about, login, registration, search, and weather. Supporting configuration files for Playwright and Node.js dependencies are added. The Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Sinatra App
participant Playwright
Developer->>GitHub Actions: Push/PR to main/dev or manual dispatch
GitHub Actions->>Sinatra App: Build and start app (test DB)
GitHub Actions->>Playwright: Install dependencies, run tests
Playwright->>Sinatra App: Simulate user actions (search, login, register, etc.)
Sinatra App-->>Playwright: Responds to simulated requests
Playwright->>GitHub Actions: Uploads HTML test report
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.gitignore (1)
58-58: Remove duplicate ignore pattern for log files.The
*.log.*pattern is already present at line 60. Keeping only one instance is sufficient and keeps the.gitignorefile clean.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/continuous_delivery.yml(1 hunks).gitignore(1 hunks)sinatra/docker-compose.prod.yml(2 hunks)sinatra/docker-entrypoint.sh(1 hunks)
🔇 Additional comments (3)
sinatra/docker-entrypoint.sh (1)
9-10: Log directory initialization and permissions are handled correctly.The script now ensures
/app/logexists and is owned byappuser, which is necessary for persistent logging in Docker.sinatra/docker-compose.prod.yml (1)
8-8: Persistent log volume is correctly configured.Mounting
sinatra_logsat/app/logand defining the volume ensures logs are persisted as intended.Also applies to: 37-37
.github/workflows/continuous_delivery.yml (1)
5-5: Workflow trigger branch list updated appropriately.The workflow will now only trigger on pushes to "main" and "dev", which matches the intended branch policy.
…and it was just confusing. Added an empty line at the bottom to adhere to best practices
Tried simplifying the hadolint steps
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/continuous_delivery.yml (1)
21-21: Remove trailing spaces for YAML cleanliness.There is a trailing space at line 21. While this does not break the workflow, it's best practice to remove unnecessary whitespace to avoid potential YAML parsing issues in stricter environments.
- with: + with:🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 21-21: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yaml(1 hunks).github/workflows/continuous_delivery.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/continuous_delivery.yml
[error] 21-21: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/continuous_delivery.yml (1)
5-5: Branch trigger update is correct.Limiting the workflow to "main" and "dev" is clear and aligns with standard CD practices.
.github/workflows/ci.yaml (2)
8-8: Addition of workflow_dispatch is a good practice.Allows manual triggering of CI, which is helpful for maintainers.
11-90: CI workflow modularization and improvements are well executed.Splitting jobs for tests, Ruby linting, and Dockerfile linting increases clarity and maintainability. Use of official actions and explicit dependency installation is robust.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sinatra/helpers/application_helpers.rb (1)
78-105: Method renaming follows Ruby conventionsRenaming from
get_cached_forecasttocached_forecastaligns with Ruby's idiomatic style, where methods that return values typically don't use aget_prefix. The implementation logic remains unchanged.The RuboCop static analysis identified this method as having high ABC complexity (Assignment Branch Condition size). Consider refactoring this method into smaller, more focused methods for better maintainability.
🧰 Tools
🪛 RuboCop (1.73)
[convention] 78-105: Assignment Branch Condition size for cached_forecast is too high. [<5, 24, 6> 25.24/23]
(Metrics/AbcSize)
[convention] 89-99: Do not use
unlesswithelse. Rewrite these with the positive case first.(Style/UnlessElse)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sinatra/app.rb(2 hunks)sinatra/benchmark_search.rb(1 hunks)sinatra/helpers/application_helpers.rb(1 hunks)sinatra/routes/api.rb(1 hunks)sinatra/routes/pages.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sinatra/benchmark_search.rb
🧰 Additional context used
🧬 Code Graph Analysis (2)
sinatra/routes/api.rb (1)
sinatra/helpers/application_helpers.rb (1)
cached_forecast(78-105)
sinatra/routes/pages.rb (1)
sinatra/helpers/application_helpers.rb (1)
cached_forecast(78-105)
🪛 RuboCop (1.73)
sinatra/helpers/application_helpers.rb
[convention] 78-105: Assignment Branch Condition size for cached_forecast is too high. [<5, 24, 6> 25.24/23]
(Metrics/AbcSize)
🔇 Additional comments (4)
sinatra/app.rb (2)
25-26: Improved session secret generation codeBreaking out the default secret generation to a separate variable improves readability while maintaining the same security level with SecureRandom.hex(32).
71-72: Improved comment placementMoving the comment about log rotation directly above the logger initialization line creates a clearer connection between the documentation and the code it describes.
sinatra/routes/api.rb (1)
45-45: Updated method call to match renamed helperThe change from
get_cached_forecasttocached_forecastmaintains consistency with the renamed helper method in application_helpers.rb.sinatra/routes/pages.rb (1)
53-53: Updated method call to match renamed helperThe change from
get_cached_forecasttocached_forecastmaintains consistency with the renamed helper method in application_helpers.rb.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
sinatra/tests/e2e/login.spec.js (2)
16-21: Consider using consistent selector approaches.While the test logic works well, there's an inconsistency in selector usage. The form rendering test uses role-based selectors, but this test uses direct CSS selectors.
For consistency, consider using role-based selectors here as well:
- await page.fill('input[name="username"]', 'wronguser'); - await page.fill('input[name="password"]', 'badpass'); - await page.click('input[type="submit"]'); + await page.getByRole('textbox', { name: /username/i }).fill('wronguser'); + await page.getByRole('textbox', { name: /password/i }).fill('badpass'); + await page.getByRole('button', { name: /log in/i }).click();
23-32: Use toHaveURL instead of direct URL comparison.The skipped test correctly handles the test case that requires seeded data. However, the URL verification could be improved.
- expect(page.url()).toBe('/'); + await expect(page).toHaveURL('/');Using
toHaveURL()is preferred as it's an async assertion that waits for the condition to be met, which is more reliable in end-to-end tests.sinatra/tests/e2e/register.spec.js (1)
3-6: Fix indentation inconsistency.The indentation in the beforeEach block uses 8 spaces while the rest of the file uses 2 spaces.
test.describe('Registration Page', () => { - test.beforeEach(async ({ page }) => { - await page.goto('/register'); - }); + test.beforeEach(async ({ page }) => { + await page.goto('/register'); + });sinatra/tests/e2e/search.spec.js (1)
33-45: Fix duplicate comment on line 42.There's a duplicate comment that should be updated to reflect the actual check being performed.
// Check the URL updates with the search term expect(page.url()).toContain(`?q=${SEARCH_TERMS.invalid}`); -// Check the URL updates with the search term +// Check that no search results are found await expect(page.locator('.search-result-title')).toHaveCount(0); await expect(page.locator('#results')).toBeEmpty();sinatra/tests/e2e/weather.spec.js (2)
8-8: Consider expanding error status code checkingThe test only accounts for HTTP status codes 200 and 503, but there might be other valid error codes like 400, 404, or 429 (rate limiting) that could be returned by the weather API.
- expect([200, 503]).toContain(status); + expect([200, 400, 404, 429, 503]).toContain(status);
39-39: Remove trailing whitespaceThere's an unnecessary trailing space after the semicolon.
- expect(itemCount).toBeGreaterThan(0); + expect(itemCount).toBeGreaterThan(0);sinatra/tests/playwright.config.js (2)
17-17: Consider making the database path configurable via environment variableThe database path is hardcoded in the command. For improved flexibility, consider reading it from an environment variable with a default fallback.
- command: 'DATABASE_PATH=./test/test_whoknows.db bundle exec rackup -p 4568', + command: 'DATABASE_PATH=${TEST_DB_PATH:-./test/test_whoknows.db} bundle exec rackup -p 4568',
38-38: Fix formatting in projects array closingThere's a formatting issue with the closing bracket of the projects array.
- },] + } + ].github/workflows/playwright.yml (3)
29-31: Consider using a dedicated SQL file for FTS table populationLine 31 contains a direct SQLite command to populate the full-text search table. For better maintainability, consider moving this to a dedicated SQL file like you did with
schema_test.sqlandfts5.sql.- sqlite3 test/test_whoknows.db "INSERT INTO pages_fts(rowid,content,language) SELECT rowid,content,language FROM pages;" + sqlite3 test/test_whoknows.db < fts_populate.sqlAnd create a new file
sinatra/fts_populate.sqlwith:INSERT INTO pages_fts(rowid,content,language) SELECT rowid,content,language FROM pages;
40-53: Decide on server startup strategyThere are commented-out sections for starting the Sinatra app and waiting for it to be ready. Since the Playwright config already handles starting the server, either remove these comments or document why they're kept.
If these are no longer needed, consider removing them to keep the workflow file clean. If they're kept for reference, add a comment explaining why.
80-86: Decide on report deployment strategyThe GitHub Pages deployment section is commented out. If this is a planned feature, add a TODO comment with the next steps needed to implement it. If it's not planned, consider removing these comments.
If you intend to implement this feature later, consider adding a TODO comment:
# TODO: Implement GitHub Pages deployment for Playwright reports once permissions are configured
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sinatra/tests/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.github/workflows/playwright.yml(1 hunks)sinatra/schema_test.sql(1 hunks)sinatra/tests/.gitignore(1 hunks)sinatra/tests/e2e/about.spec.js(1 hunks)sinatra/tests/e2e/login.spec.js(1 hunks)sinatra/tests/e2e/register.spec.js(1 hunks)sinatra/tests/e2e/search.spec.js(1 hunks)sinatra/tests/e2e/weather.spec.js(1 hunks)sinatra/tests/package.json(1 hunks)sinatra/tests/playwright.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- sinatra/tests/.gitignore
- sinatra/tests/package.json
- sinatra/schema_test.sql
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e
- GitHub Check: e2e
🔇 Additional comments (10)
sinatra/tests/e2e/about.spec.js (1)
1-8: LGTM! Well-structured test for the about page.This test effectively validates the critical elements of the about page: title, mission heading, and descriptive text. The selectors used are appropriate and follow Playwright best practices.
sinatra/tests/e2e/login.spec.js (1)
4-14: LGTM! Well-structured test setup and form rendering test.Good organization with test.beforeEach for the login page navigation. The form rendering test uses appropriate role-based selectors to verify all form elements.
sinatra/tests/e2e/register.spec.js (3)
8-18: LGTM! Well-structured registration form test.The form rendering test appropriately checks for all required elements with clear sections for the heading, inputs, and submit button.
20-30: LGTM! Good validation test for password mismatch.This test effectively validates the password mismatch error case with appropriate error message verification.
32-42: LGTM! Strong approach to testing successful registration.Using
Date.now()to generate unique usernames is a good practice to avoid test data conflicts. The redirect and success message verification provide good end-to-end validation.sinatra/tests/e2e/search.spec.js (2)
8-11: LGTM! Good use of constants for search terms.Defining search terms as constants improves readability and maintainability.
15-31: LGTM! Comprehensive test for valid search results.The test covers all the critical aspects of searching with a valid term: URL parameters, result count, and content verification.
sinatra/tests/e2e/weather.spec.js (1)
1-42: Well-structured test suite with comprehensive error handlingThe test suite effectively covers both the API and UI aspects of the weather feature, handling both success and error scenarios appropriately. This is excellent test coverage.
sinatra/tests/playwright.config.js (1)
1-39: Well-configured Playwright setup with cross-browser testingThe configuration properly sets up parallel test execution, CI-specific settings, and multiple browser targets. Good job including all three major browser engines.
.github/workflows/playwright.yml (1)
1-87: Well-structured GitHub Actions workflow for E2E testingThe workflow correctly sets up all necessary dependencies and configurations for running the Playwright tests, including database preparation and artifact uploading.
| # permissions: | ||
| # contents: write # Needed for deploying to GitHub Pages |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Decide on GitHub Pages deployment strategy
The permissions section is commented out. If you plan to deploy reports to GitHub Pages in the future, consider either removing these comments or implementing the feature properly.
🌐 Web query:
How to deploy Playwright reports to GitHub Pages?
💡 Result:
How to Deploy Playwright Reports to GitHub Pages
Deploying Playwright HTML reports to GitHub Pages is a practical way to make your browser test results easily accessible to your team or stakeholders. Below is a step-by-step guide to automate this process using GitHub Actions.
1. Enable GitHub Pages on Your Repository
- Create a
gh-pagesbranch (you can do this manually or let an action manage it)[2][5]. - In the repository settings, enable GitHub Pages and select the
gh-pagesbranch as the source[2][5].
2. Use the Playwright HTML Reporter
-
Ensure you are using Playwright version 1.17.0 or newer, as the HTML reporter generates a static site[5][6].
-
In your test command, specify the HTML reporter:
npx playwright test --reporter=htmlThis creates a
playwright-reportdirectory containing static HTML files[5][6].
3. Set Up a GitHub Actions Workflow
Add or update a GitHub Actions workflow .github/workflows/playwright.yml to:
- Install dependencies and browsers
- Run Playwright tests and generate the report
- Deploy the
playwright-reportfolder to GitHub Pages using the popular peaceiris/actions-gh-pages GitHub Action[1][5].
Example workflow snippet:
name: Playwright Tests and Publish Report
on:
push:
branches: [main]
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Setup Node.js
uses: actions/setup-node@v3
with:
node-version: 18
- name: Install dependencies
run: npm ci
- name: Install Playwright browsers
run: npx playwright install --with-deps
- name: Run Playwright tests
run: npx playwright test --reporter=html
- name: Deploy report to GitHub Pages
uses: peaceiris/actions-gh-pages@v3
with:
github_token: ${{ secrets.GITHUB_TOKEN }}
publish_dir: ./playwright-reportThis workflow:
- Runs the tests and generates
playwright-report - Publishes
playwright-reportto thegh-pagesbranch, updating your GitHub Pages site[1][5]
4. Accessing Your Report
After the workflow runs, your report will be accessible at:
https://<username>.github.io/<repository>/
You can further organize reports by build number, branch, or run ID by customizing the publish_dir or artifacts paths[1][2].
Additional Tips
- You can use GitHub Actions to display the report URL in the job summary for convenient sharing[1][2].
- For merging reports from different shards or jobs, use
npx playwright merge-reportsbefore publishing[1]. - The HTML report supports filters and trace viewing directly in the browser[6].
Summary Table
| Step | Command/Action |
|---|---|
| Enable GitHub Pages | Set source branch to gh-pages |
| Generate Playwright HTML report | npx playwright test --reporter=html |
| Deploy with GitHub Actions | Use peaceiris/actions-gh-pages to publish playwright-report |
| Access report | https://<username>.github.io/<repo>/ |
This approach ensures that your Playwright test reports are always available as a static site, making test results transparent and easily distributable[1][2][5][6][7].
Citations:
- 1: https://blog.martioli.com/publish-your-playwright-reports-to-github-pages/
- 2: https://ysfaran.github.io/blog/2022/09/02/playwright-gh-action-gh-pages/
- 3: https://www.youtube.com/watch?v=F_nUVHBhrow
- 4: https://playwright.dev/docs/ci-intro
- 5: https://marcusfelling.com/blog/2021/publishing-playwright-test-results-to-github-pages/
- 6: https://www.restack.io/p/playwright-report-github-pages-answer
- 7: https://www.workwithloop.com/blog/playwright-github-pages
- 8: https://josdem.io/techtalk/ux/playwright_reports_deployment/
Enable GitHub Pages deployment in your Playwright workflow
To push the generated HTML report to GitHub Pages you’ll need to:
• Un‑comment and configure permissions at the top of .github/workflows/playwright.yml
• Install and use a “deploy to gh‑pages” action (e.g. peaceiris/actions-gh-pages) after npx playwright test --reporter=html
Suggested changes:
name: Playwright Tests and Publish Report
+permissions:
+ contents: write # allow pushing to gh-pages
on:
push:
branches: [main]
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
@@
- name: Run Playwright tests
run: npx playwright test --reporter=html
+ - name: Deploy report to GitHub Pages
+ uses: peaceiris/actions-gh-pages@v3
+ with:
+ github_token: ${{ secrets.GITHUB_TOKEN }}
+ publish_dir: ./playwright-report• If you’re not ready to publish reports yet, remove the stale comments entirely.
• Otherwise add the full deployment job as shown above so your reports land on the gh-pages branch and appear at https://.github.io//.
add end2end report to github pages
Revert "add end2end report to github pages"
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/playwright.yml (1)
11-13: Decision needed: GitHub Pages deploymentThe permissions section for GitHub Pages deployment is commented out. Either implement the feature or remove these comments.
This was mentioned in a previous review. Please decide whether to:
- Fully implement the GitHub Pages reporting feature (uncomment this section and lines 80-86)
- Remove all commented GitHub Pages related code for clarity
🧹 Nitpick comments (9)
.github/workflows/continuous_delivery.yml (1)
21-21: Remove trailing whitespace.There's trailing whitespace after "with:" which should be removed to maintain clean YAML formatting.
- with: + with:🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 21-21: trailing spaces
(trailing-spaces)
sinatra/helpers/application_helpers.rb (1)
78-105: Consider breaking down the method for improved maintainability.The method has a high complexity score (ABC size of 25.24/23). Consider refactoring into smaller helper methods for better maintainability and testability.
For example:
# Retrieves forecast data, using cache if valid, otherwise fetches fresh data. def cached_forecast cache = settings.forecast_cache expiration = settings.forecast_cache_expiration # Check cache validity using UTC time for comparison # Use Time.now.utc for consistent time zone handling if cache.nil? || expiration < Time.now.utc - logger.info "Weather forecast cache miss or expired. Fetching new data." - new_forecast = fetch_forecast - - # Update cache only if the fetch was successful (no 'error' key) - unless new_forecast.key?('error') - settings.forecast_cache = new_forecast - # Use Time.now.utc when setting the new expiration time - settings.forecast_cache_expiration = Time.now.utc + 3600 # 1 hour cache - logger.info "Weather forecast cache updated." - return new_forecast # Return the newly fetched data - else - # Log failure but return the error hash from fetch_forecast - logger.warn "Failed to fetch new forecast data. Returning error." - return new_forecast - end + fetch_and_update_cache else # Cache hit, return cached data logger.debug "Weather forecast cache hit." return cache end end + + # Helper method to fetch and update the forecast cache + def fetch_and_update_cache + logger.info "Weather forecast cache miss or expired. Fetching new data." + new_forecast = fetch_forecast + + # Update cache only if the fetch was successful (no 'error' key) + unless new_forecast.key?('error') + update_forecast_cache(new_forecast) + else + # Log failure but return the error hash from fetch_forecast + logger.warn "Failed to fetch new forecast data. Returning error." + end + new_forecast + end + + # Helper method to update the forecast cache + def update_forecast_cache(forecast) + settings.forecast_cache = forecast + # Use Time.now.utc when setting the new expiration time + settings.forecast_cache_expiration = Time.now.utc + 3600 # 1 hour cache + logger.info "Weather forecast cache updated." + end🧰 Tools
🪛 RuboCop (1.73)
[convention] 78-105: Assignment Branch Condition size for cached_forecast is too high. [<5, 24, 6> 25.24/23]
(Metrics/AbcSize)
[convention] 89-99: Do not use
unlesswithelse. Rewrite these with the positive case first.(Style/UnlessElse)
sinatra/schema_test.sql (1)
15-21: Consider adding search indexes to the pages table.Since this project is focused on search functionality (DynaSearch), consider adding full-text search indexes to the pages table to improve search performance.
For SQLite, you could add:
-- Add FTS5 virtual table for searching pages CREATE VIRTUAL TABLE IF NOT EXISTS pages_fts USING fts5( title, content, content='pages', tokenize='porter unicode61' ); -- Create triggers to keep FTS table in sync CREATE TRIGGER IF NOT EXISTS pages_ai AFTER INSERT ON pages BEGIN INSERT INTO pages_fts(rowid, title, content) VALUES (new.rowid, new.title, new.content); END;sinatra/tests/e2e/search.spec.js (1)
33-45: Duplicate comment on lines 39-42Line 42 repeats the same comment as line 39, but the actions in lines 43-44 are actually checking for no results.
// Check the URL updates with the search term expect(page.url()).toContain(`?q=${SEARCH_TERMS.invalid}`); - // Check the URL updates with the search term + // Verify no results are displayed await expect(page.locator('.search-result-title')).toHaveCount(0); await expect(page.locator('#results')).toBeEmpty();sinatra/tests/playwright.config.js (1)
16-21: Web server configuration uses hardcoded pathsThe database path and port are hardcoded in the command. Consider using environment variables for these values to make the configuration more flexible.
webServer: { - command: 'DATABASE_PATH=./test/test_whoknows.db bundle exec rackup -p 4568', + command: `DATABASE_PATH=${process.env.TEST_DB_PATH || './test/test_whoknows.db'} bundle exec rackup -p ${process.env.TEST_PORT || 4568}`, url: 'http://127.0.0.1:4568', cwd: '../', reuseExistingServer: !process.env.CI, },sinatra/tests/e2e/weather.spec.js (1)
25-41: Well-implemented UI test with conditional assertionsThe UI test properly handles both success and error states of the weather page by:
- Verifying the heading is always visible
- Conditionally testing either the error message or forecast items
This makes the test resilient to different API states without requiring mocks.
- expect(itemCount).toBeGreaterThan(0); + expect(itemCount).toBeGreaterThan(0);.github/workflows/playwright.yml (3)
33-39: Remove trailing whitespaceThere's a trailing space after
bundler-cache: trueon line 37.- bundler-cache: true + bundler-cache: true🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 37-37: trailing spaces
(trailing-spaces)
40-54: Decide on Sinatra app startup approachThese commented steps for starting the Sinatra app suggest you've chosen a different approach (likely in the Playwright config). Consider removing these comments if they're no longer needed to keep the workflow file clean.
80-86: Clean up GitHub Pages deployment commentsThe commented GitHub Pages deployment configuration contains doubly-commented lines (
# #), indicating indecision or confusion. Either:
- Implement this feature completely (remove comments and configure correctly)
- Remove these lines entirely to keep the workflow file clean
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sinatra/tests/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.github/workflows/ci.yaml(1 hunks).github/workflows/continuous_delivery.yml(2 hunks).github/workflows/playwright.yml(1 hunks).gitignore(1 hunks)sinatra/app.rb(2 hunks)sinatra/benchmark_search.rb(1 hunks)sinatra/docker-compose.prod.yml(2 hunks)sinatra/docker-entrypoint.sh(1 hunks)sinatra/helpers/application_helpers.rb(1 hunks)sinatra/routes/api.rb(1 hunks)sinatra/routes/pages.rb(1 hunks)sinatra/schema_test.sql(1 hunks)sinatra/tests/.gitignore(1 hunks)sinatra/tests/e2e/about.spec.js(1 hunks)sinatra/tests/e2e/login.spec.js(1 hunks)sinatra/tests/e2e/register.spec.js(1 hunks)sinatra/tests/e2e/search.spec.js(1 hunks)sinatra/tests/e2e/weather.spec.js(1 hunks)sinatra/tests/package.json(1 hunks)sinatra/tests/playwright.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- sinatra/benchmark_search.rb
- sinatra/routes/api.rb
- sinatra/tests/.gitignore
- sinatra/tests/package.json
- sinatra/routes/pages.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- sinatra/docker-entrypoint.sh
- .gitignore
- sinatra/app.rb
- sinatra/docker-compose.prod.yml
- .github/workflows/ci.yaml
🧰 Additional context used
🧬 Code Graph Analysis (3)
sinatra/tests/playwright.config.js (5)
sinatra/tests/e2e/login.spec.js (1)
require(1-1)sinatra/tests/e2e/about.spec.js (1)
require(1-1)sinatra/tests/e2e/search.spec.js (1)
require(1-1)sinatra/tests/e2e/register.spec.js (1)
require(1-1)sinatra/tests/e2e/weather.spec.js (1)
require(1-1)
sinatra/tests/e2e/search.spec.js (5)
sinatra/tests/e2e/login.spec.js (1)
require(1-1)sinatra/tests/e2e/about.spec.js (1)
require(1-1)sinatra/tests/e2e/register.spec.js (1)
require(1-1)sinatra/tests/playwright.config.js (1)
require(2-2)sinatra/tests/e2e/weather.spec.js (1)
require(1-1)
sinatra/tests/e2e/weather.spec.js (5)
sinatra/tests/e2e/login.spec.js (1)
require(1-1)sinatra/tests/e2e/about.spec.js (1)
require(1-1)sinatra/tests/e2e/search.spec.js (1)
require(1-1)sinatra/tests/e2e/register.spec.js (1)
require(1-1)sinatra/tests/playwright.config.js (1)
require(2-2)
🪛 YAMLlint (1.35.1)
.github/workflows/playwright.yml
[error] 37-37: trailing spaces
(trailing-spaces)
.github/workflows/continuous_delivery.yml
[error] 21-21: trailing spaces
(trailing-spaces)
🪛 RuboCop (1.73)
sinatra/helpers/application_helpers.rb
[convention] 78-105: Assignment Branch Condition size for cached_forecast is too high. [<5, 24, 6> 25.24/23]
(Metrics/AbcSize)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
🔇 Additional comments (18)
.github/workflows/continuous_delivery.yml (1)
4-5: Appropriate update to the workflow trigger branches.Switching from "feature/refactor" to "dev" for the continuous delivery workflow is a good change that aligns with standard Git workflow practices.
sinatra/helpers/application_helpers.rb (1)
78-78: Good method naming improvement.Renaming from
get_cached_forecasttocached_forecastfollows Ruby conventions by removing the redundant "get_" prefix while maintaining clarity.🧰 Tools
🪛 RuboCop (1.73)
[convention] 78-105: Assignment Branch Condition size for cached_forecast is too high. [<5, 24, 6> 25.24/23]
(Metrics/AbcSize)
sinatra/tests/e2e/about.spec.js (1)
3-8: Well-structured Playwright test.This test effectively checks the critical elements of the about page. Good use of Playwright's role-based selectors and appropriate assertions.
sinatra/tests/e2e/login.spec.js (3)
1-8: Well-structured test setup with clear navigationThe test organization with a
beforeEachhook that navigates to the login page before each test is a good practice that reduces code duplication and ensures consistent test preconditions.
9-14: Good use of role-based and attribute selectorsUsing a combination of role-based selectors (getByRole) and attribute selectors (locator) follows Playwright best practices and makes tests more resilient to UI implementation changes.
16-21: Valid error case testingTesting the error scenario with invalid credentials is important for verifying proper user feedback. The test correctly checks for the presence of an error message.
sinatra/tests/e2e/search.spec.js (2)
1-11: Good use of constants for test dataDefining search terms as constants at the top of the file improves maintainability and makes it clear what values are being tested.
14-31: Well-structured positive test caseThe test properly validates multiple aspects of the search functionality, including URL updates and result content verification. The use of case-insensitive comparison in line 29 is a good practice.
sinatra/tests/playwright.config.js (2)
1-14: Well-configured Playwright settings with CI awarenessThe configuration effectively handles different environments (CI vs local) with appropriate settings for retries, parallel execution, and reporting. This ensures tests run reliably in both scenarios.
23-39: Good cross-browser test coverageTesting across multiple browsers (Chromium, Firefox, WebKit) ensures the application works consistently for different users. The use of Playwright's device configurations simplifies this setup.
sinatra/tests/e2e/register.spec.js (3)
1-18: Comprehensive form validation testThe test thoroughly checks all form elements, which helps ensure the UI renders correctly. The combination of role-based and attribute selectors follows Playwright best practices.
20-30: Good validation of error handlingTesting the password mismatch scenario is important for verifying error handling. The test properly checks both the visibility and content of the error message.
32-42: Excellent use of unique data for registration testUsing timestamp-based unique values for username and email prevents test data collisions and potential test failures due to duplicate registrations. This is a best practice for registration tests.
sinatra/tests/e2e/weather.spec.js (2)
1-3: Good implementation of Playwright test structure!The test suite is well-structured with proper imports and test organization, matching the pattern used in other test files.
4-23: Well-designed API test with thorough error handlingThis test effectively validates both success and error paths:
- Properly checks for both 200 and 503 status codes
- For success: Verifies expected properties and data structure
- For errors: Confirms proper error format with status and message
The conditional testing logic ensures robust validation regardless of the API state.
.github/workflows/playwright.yml (3)
1-9: Well-configured workflow triggersThe workflow is properly set up to run on:
- Pushes to dev/main branches
- Pull requests to dev/main branches
- Manual workflow dispatch
This ensures comprehensive test coverage at appropriate points in the development cycle.
26-32: Well-structured database preparationThe test database setup is comprehensive:
- Applies schema from SQL files
- Sets up full-text search
- Populates the virtual table
This ensures tests run against a properly configured database environment.
66-71: Good security practice with API keyProperly passing the WeatherBit API key as an environment variable from GitHub secrets follows security best practices. This ensures the key isn't exposed in logs or code.
| -- Create a default user, The password is 'password' (MD5 hashed) | ||
| INSERT INTO users (username, email, password) | ||
| VALUES ('test', 'keamonk1@stud.kea.dk', '123'); |
There was a problem hiding this comment.
Fix password handling in test user.
There are issues with the test user's password:
- The comment states the password is MD5 hashed, but '123' appears to be plaintext
- MD5 is insecure for password storage even if it were used
While this may be acceptable for testing, ensure production code uses secure password hashing (like bcrypt, which I see is used in the application helpers).
-- Create a default user, The password is 'password' (MD5 hashed)
+-- Create a default user with a plaintext password (for testing only)
INSERT INTO users (username, email, password)
VALUES ('test', 'keamonk1@stud.kea.dk', '123');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- Create a default user, The password is 'password' (MD5 hashed) | |
| INSERT INTO users (username, email, password) | |
| VALUES ('test', 'keamonk1@stud.kea.dk', '123'); | |
| -- Create a default user with a plaintext password (for testing only) | |
| INSERT INTO users (username, email, password) | |
| VALUES ('test', 'keamonk1@stud.kea.dk', '123'); |
| // Requires seeded user: username 'test', password '123' | ||
| test.skip('allows successful login', async ({ page }) => { | ||
| await page.fill('input[name="username"]', 'test'); | ||
| await page.fill('input[name="password"]', '123'); | ||
| await page.click('input[type="submit"]'); | ||
|
|
||
| // Verify redirect to home | ||
| expect(page.url()).toBe('/'); | ||
| await expect(page.getByText('You were successfully logged in.')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding a data seeding mechanism
The skipped test for successful login requires seeded test data, but there's no clear indication of how or where this data should be created.
🏁 Script executed:
#!/bin/bash
# Check if there's a database seeding script or fixture in the codebase
fd -t f "seed|fixture" --full-path sinatra/
# Check for any references to test user creation in the codebase
rg -t ruby "test.*user|fixture.*user|seed.*user" sinatra/Length of output: 435
Add a data‑seeding mechanism for the “allows successful login” E2E test
This test is currently skipped because it depends on a pre‑existing user (test/123), but there’s no fixture or seed script to create that account before Playwright runs.
Please add a clear seeding step—for example, a Playwright fixture or a standalone scripts/seed entrypoint—that inserts the test user into the database prior to executing this spec.
• Location: sinatra/tests/e2e/login.spec.js (lines 23–32)
• Suggestion:
– Create a seedTestUser() helper (via direct DB call or API endpoint) and invoke it in a Playwright beforeAll or custom fixture.
– Alternatively, add a CLI script (scripts/seed.js or seeds/) that your CI/test runner can invoke to populate test data.
• Verify that running the seed step enables this test to pass reliably.
|
Nice! Looking forward to see how it works |
3 participants
Description
Added loggin for search, and made so that this gets saved in a Docker volume. Added the log files to gitignore, so these doesn't get shared to the public. Critical business information hihi
Related Issue:
If applicable, please reference the issue number that this pull request fixes:
Fixes #124 #137
Type of Change
Please check the type(s) of changes your code introduces:
Checklist
Before creating this pull request, please ensure that your code meets the following requirements:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
.gitignorefor test artifacts.Refactor
Revert