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

Run cypress #1479

Closed
Closed

Conversation

kinow
Copy link
Collaborator

@kinow kinow commented Aug 1, 2023

Reasons for creating this PR

Run Cypress in GitHub Actions.

Link to relevant issue(s), if any

Description of the changes in this PR

Hi, this builds on top of #1477 and #1474. The first switches to Apache Jena Fuseki image (which is a lot simpler to run as it doesn't have a bug with volumes and restarting the process). The second PR is adding some Cypress tests, so I used that one as base here for running some tests.

image

Known problems or uncertainties in this PR

Out of the 7 tests in #1474, one is failing due to the YSO not being included with the Docker Compose recipe. On my working copy, I started writing some code to download yso.ttl in GH Actions, and then load the dataset into the running Docker container. But I decided to wait to see if that Cypress test will be kept or not.

Instead of running the actual dataset, with Cypress we can also mock the calls and pretend Skosmos replied something, which saves us from having to load the dataset. But since we have a running instance with Docker compose, we can do it whichever way works best for others -- https://docs.cypress.io/api/commands/intercept#Usage

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)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 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 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

# prefix: /app
#
# - name: Publish code coverage to Codecov
# uses: codecov/codecov-action@v3
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Disabling just to run CI a bit faster. Will add it back later.

- name: Install
run: yarn install
- name: Start Skosmos
run: cd dockerfiles && docker compose up -d
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here before we run the tests, we can have a step that loads datasets, if necessary.

Startup of Skosmos is taking ~2.5 minutes on GH Actions with Docker Compose. So the overall execution for Cypress is quite good for now.

@@ -2,7 +2,7 @@ const { defineConfig } = require("cypress");

module.exports = defineConfig({
e2e: {
baseUrl: 'http://localhost/Skosmos',
baseUrl: 'http://localhost:9090/',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can pass that via command line too. If most devs will use their own localhost/Skosmos instead of docker or 9090, that might make more sense. Changed here as that was the simplest to get the Cypress tests executed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ignore these dockerfiles modified here. I just cherry-picked from #1477

@osma osma added this to the 3.0 milestone Aug 17, 2023
@osma osma self-assigned this Aug 17, 2023
@osma
Copy link
Member

osma commented Aug 17, 2023

Thanks a lot @kinow , I will review this soon!

@osma
Copy link
Member

osma commented Aug 22, 2023

This looks very promising!

A few comments:

Vocabulary data files

One of the tests failed because YSO isn't available. This is not a surprise and not the fault of this PR.

We have discussed how to set up the Skosmos and Fuseki environment for the Cypress tests. The challenge is that this should be standardized as much as possible, so that tests are running in a similar environment both under CI and locally on developer machines. This is basically the current plan:

  1. Use Docker for running Fuseki (which you've done here, great!)
  2. Use the same Skosmos configuration which is used for the PHPUnit tests: tests/testconfig.ttl
  3. Load the same minimal vocabulary data files as used by PHPUnit tests. These are currently loaded from data files undertests/test-vocab-data/ using tests/init_fuseki.sh which relies on s-put, but for Dockerized Fuseki probably curl would be better as it's usually available.

This way we can write Cypress tests knowing that Skosmos is configured in a standard way and Fuseki contains a specific, known set of vocabulary data.

For this PR, 1. is already done, 2. requires using tests/testconfig.ttl instead of config/config-docker-compose.ttl as the Skosmos configuration file when running docker-compose, and 3. requires adding a script that loads the test vocabularies to Fuseki using curl, similar to the init_fuseki.sh script. Also, the current Cypress test that relies on YSO being available must be adjusted, because YSO is not among the test vocabularies (they are all very small).

Installing node.js packages

I noticed you've used yarn to install Node.js dependencies and committed the yarn.lock file. So far we've been using npm instead and deliberately avoided committing package-lock.json to version control - see around line 58 in ci.yml and the current .gitignore. I think this approach should be harmonized, so we should decide on either yarn or npm (opinions?) and also whether to have the lock file under version control or not. So far we've avoided this (also for Composer) and preferred to manage approximate version numbers in package.json (and composer.json).

Cleaning up before merging

This is probably obvious, but clarifying just in case:

@osma
Copy link
Member

osma commented Sep 20, 2023

I decided to reimplement the Action that runs Cypress under CI rather than continuing from this PR, because it was easier to start from the current state of the skosmos-3 branch which has changed a lot since this PR was opened.
So this PR is now superseded by PR #1518 which will be merged Very Soon (tm). Closing this one.
Thanks for the great example @kinow , I used this as inspiration for the new PR, just couldn't reuse the code.

@osma osma closed this Sep 20, 2023
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