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

Use dockerized Skosmos for Cypress tests #1509

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

osma
Copy link
Member

@osma osma commented Aug 31, 2023

Reasons for creating this PR

We want to run Cypress tests in a standard environment. See #1508

Link to relevant issue(s), if any

Description of the changes in this PR

  • define a custom tests/docker-compose.yml that extends dockerfiles/docker-compose.yml and overrides the skosmos configuration, so that Skosmos in the container uses the same testconfig.ttl configuration as the PHPUnit tests
  • rename init_fuseki.sh script to init_containers.sh because it initializes also a Skosmos container; and make it use the customized docker-compose.yml from above
  • make a separate skosmos configuration file for PHPUnit tests that involve overriding baseHref, and drop the baseHref setting from the regular testconfig.ttl (otherwise links aren't working in the skosmos container)
  • configure Cypress to use the dockerized skosmos on localhost:9090
  • (unrelated) fix the Cypress test for the error page that was failing due to a change in template HTML structure

After these changes, I can successfully run both the PHPUnit and Cypress tests locally. Instructions for running the tests:

Set up the Docker containers:

cd tests
./init_containers.sh

Run the tests normally:

vendor/bin/phpunit
npx cypress open # or run

After running the tests, the containers can be shut down like this:

cd tests
docker compose down

Known problems or uncertainties in this PR

Wiki documentation for running PHPUnit and Cypress tests should be updated when/after merging this.

This PR doesn't include running Cypress tests under GitHub Actions CI; we have a draft PR #1479 about that, but it needs some more work that is best done after this PR is merged.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma self-assigned this Aug 31, 2023
@osma osma added this to the 3.0 milestone Aug 31, 2023
@osma osma force-pushed the issue1508-cypress-dockerized-skosmos branch from a9898cd to e7b2330 Compare August 31, 2023 13:46
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (cfedf5d) 70.09% compared to head (cfedf5d) 70.09%.

❗ Current head cfedf5d differs from pull request most recent head 7f57250. Consider uploading reports for the commit 7f57250 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             skosmos-3    #1509   +/-   ##
============================================
  Coverage        70.09%   70.09%           
  Complexity        1642     1642           
============================================
  Files               32       32           
  Lines             4314     4314           
============================================
  Hits              3024     3024           
  Misses            1290     1290           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@osma osma changed the base branch from skosmos-3 to master August 31, 2023 16:26
@osma osma changed the base branch from master to skosmos-3 August 31, 2023 16:26
@osma osma force-pushed the issue1508-cypress-dockerized-skosmos branch from e7b2330 to 7f57250 Compare September 1, 2023 08:27
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@osma
Copy link
Member Author

osma commented Sep 1, 2023

I merged #1496 into skosmos-3, then rebased this PR branch on top of current skosmos-3 to clean up the commit history and diff. Now this PR shows only actual changes w.r.t. current skosmos-3.

@osma osma requested a review from miguelvaara September 1, 2023 08:29
Copy link
Contributor

@miguelvaara miguelvaara left a comment

Choose a reason for hiding this comment

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

When running the command ./init_containers.sh, I encountered an error:

Error response from daemon: Conflict. The container name "/skosmos-fuseki-cache" is already in use by container "0fcc35ef7ecc3336b294cb2e577de388e77e24885a866cf2a88c1b1489b9314c". You have to remove (or rename) that container to be able to reuse that name.
Waiting for Fuseki to get ready
...waiting...
...waiting...
...waiting...

I removed the previous containers (deleting just the Varnish container would likely have sufficed), after which all runs went smoothly. I am wondering whether this situation could be considered problematic or not. I was considering how much effort it would be to implement some adjustment that automatically removes a conflicting container. On the other hand, manually removing the conflicting container and re-running the script is not a significant task. In my opinion, both approaches are acceptable; having automation or having the tester manually remove the conflicting container.

However, the issue was a one-time occurrence and cannot be reproduced anymore, so there is no need to focus on it any further.

Copy link
Contributor

@miguelvaara miguelvaara left a comment

Choose a reason for hiding this comment

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

The execution of PHPUnit tests was successful. Of course, the tests reported errors, but that's okay, and the passing of tests is not within the scope of this PR.

!! The errors in PHPUnit tests were my own mistake. I forgot to run php composer update - once again :-D

Copy link
Contributor

@miguelvaara miguelvaara left a comment

Choose a reason for hiding this comment

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

The Cypress tests are working well, meaning that the modified settings are functioning and the http://localhost:9090 URL is responding correctly.

Copy link
Contributor

@miguelvaara miguelvaara left a comment

Choose a reason for hiding this comment

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

Everything seems to be working well. In think we can proceed with the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

2 participants