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.

Sign up for GitHub

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

Jump to bottom

Add basic tests #388

Open
thewilkybarkid opened this issue Jul 22, 2021 · 0 comments
Open

Add basic tests #388

thewilkybarkid opened this issue Jul 22, 2021 · 0 comments
Assignees
@thewilkybarkid

Comments

@thewilkybarkid
Copy link
Member

Since the codebase currently has no tests, they'll need to be written before making changes to reduce the chances of regression.

To start with, it'd be useful to write a few basic tests to get the runner(s) working.

Jest is already installed, but I'd like to also try Playwright for system tests.
@thewilkybarkid thewilkybarkid self-assigned this Jul 22, 2021
thewilkybarkid added a commit that referenced this issue Jul 27, 2021
@thewilkybarkid
ci(docker): separate testing and linting from the build
7cca910 
Creates new CI steps to run the lint and test commands, rather than having them run inside the Docker build. This means
that the image can be built even if there are linting issues (which will be useful for local development), and that the
tests will be able to make use of other processes (and given the app has a reliance on React and Postgres, this is
vital).

Refs #388, #390
thewilkybarkid added a commit that referenced this issue Jul 28, 2021
@thewilkybarkid
test(integration): make sure the homepage looks correct
c217be1 
Adds a visual-regression test for the homepage using Playwright: this makes sure that it looks the same in Chromium on
a large screen.

It blocks requests to Twitter to stop the timeline in the footer from changing, and resets both the carousels to the
first item.

It uses Git LFS to store the reference screenshots, since they might be quite large.

Refs #388
thewilkybarkid added a commit that referenced this issue Jul 28, 2021
@thewilkybarkid
fix(git-lfs): fix Git LFS usage
e80293d 
Husky prevented the Git LFS pre-push hook from working leading to the screenshot not being uploaded. This removes Husky
entirely since it's problematic, and a bit broken (files getting fixed randomly after committing). GitHub Actions also
needs to be told to download Git LFS items.

Refs #388
thewilkybarkid added a commit that referenced this issue Jul 28, 2021
@thewilkybarkid
test(integration): use an emulated mobile device
f78d275 
Runs the integration tests on an emulated iPhone 11.

The reference image does contain artefacts in the cards which I can't replicate on either mobile or desktop Safari.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 2, 2021
@thewilkybarkid
test(integration): allow retries
d666baa 
The integration tests seem to have nondeterministic failures regarding antialiasing; this allows
the test runner to retry 3 times to reduce the number of failed builds.

Refs #388, microsoft/playwright#7548
thewilkybarkid added a commit that referenced this issue Aug 3, 2021
@thewilkybarkid
test(integration): regenerate reference image
39fa33b 
Not clear why there are some antialiasing difference in Chromium, but as this has started to happen
locally too I can regenerate it.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 3, 2021
@thewilkybarkid
test(integration): blur screenshots before comparison
d36ae9d 
I've spent quite a while trying various options to reduce the number of failures due to
antialiasing differences, without success. This takes a bit more of an extreme option to blur the
screenshot slightly to try and normalise it. The goal of the test isn't pixel-precision, so this
should be ok.

Ideally, this will be a feature in Playwright (so the reference image doesn't have to be blurred
too).

Refs #388, microsoft/playwright#7548
thewilkybarkid added a commit that referenced this issue Aug 4, 2021
@thewilkybarkid
test(integration): cover searching for non-existent preprints and PRE…
453b948 
…reviewers

Adds specs searching for terms and names that should not return any results.

There are currently no fixtures; I think that the reference screenshots will need to be updated
when added as the display does change (e.g. filter options appear).

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 4, 2021
@thewilkybarkid
test(integration): cover being asked to log in when requesting a review
0a7ae8f 
Adds a spec making sure that the log-in dialog appears when trying to request a review.

I've not gone further for the moment as it requires integration with an external system (ORCID).

Ref #388
thewilkybarkid added a commit that referenced this issue Aug 4, 2021
@thewilkybarkid
test(integration): cover starting a community
f6e2fc5 
Add a spec making sure that the 'start your own community' button goes to the correct form.

Since it's an external Google Form, I've not added a screenshot.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 4, 2021
@thewilkybarkid
test(integration): disable CSS transition before taking a screenshot
c4d06c4 
The log-in dialog has a slight fade-in transition, which can lead to a race condition when taking a
screenshot. This forcibly removes all transitions on the page.

Ref #388
thewilkybarkid added a commit that referenced this issue Aug 4, 2021
@thewilkybarkid
test(integration): extract screenshot helper
60207a5 
Adds a helper function that hides the normalisation steps when taking a screenshot.

It also takes an optional focus node which puts it into view if present, otherwise the screenshot
covers the whole page.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 5, 2021
@thewilkybarkid
test(integration): hide rather than block the Twitter timeline
3f1c541 
Using page.route() prevents browser caching from working, which will help speed up the integration
tests (once resources are allowed to be cached). This instead hides the Twitter timeline from the
screenshot. This has a slight difference in that the space it occupies is present, hence the
difference in the iPhone homepage reference image.

This does mean that the Twitter timeline needs a consistent space, whether it's loaded yet or not.
This is done by moving the height from the timeline to its parent element.

I'm not sure why the requesting-a-review reference image also changes; I've not been able to
recreate the 2-line button effect locally. It does now look more like the existing iPhone image
though.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 6, 2021
@thewilkybarkid
build(docker): allow the app to authenticate with the ORCID sandbox
79b8db0 
This sets up the environment variables required to log in with ORCID when running the app locally.

It relies on having a valid PREREVIEW_ORCID_CLIENT_ID and PREREVIEW_ORCID_CLIENT_SECRET in the
local environment.

Refs #388, #390
thewilkybarkid added a commit that referenced this issue Aug 6, 2021
@thewilkybarkid
test(integration): add extra diagnostics
df149a9 
Configures Playwright to record browser traces and a video, which can help provide more context as
to why a test has failed.

Refs #388
@thewilkybarkid thewilkybarkid mentioned this issue Aug 10, 2021
Open
thewilkybarkid added a commit that referenced this issue Aug 12, 2021
@thewilkybarkid
build(make): remove containers after running
4105d3a 
Docker Compose keeps containers and volumes after running by default. The integrations tests need a clean database, so running an environment beforehand can see indeterminate results. Rather than cleaning the database before starting, this removes the containers after running. It also removes the persistent database, which has little value.

Refs #388, #395
thewilkybarkid added a commit that referenced this issue Aug 13, 2021
@thewilkybarkid
ci(integration): record Docker logs
68899fc 
When debugging integration test failures it's useful to see what the server did, so this writes all the Docker logs to a file.

Refs #388, #395
thewilkybarkid added a commit that referenced this issue Aug 16, 2021
@thewilkybarkid
test(integration): add an admin user with an API key
10b89bd 
Adds a new environment for the integration tests, which includes an admin user with an API key. This will allow using the API to set up fixtures.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 16, 2021
@thewilkybarkid
test(integration): wait for the page to load before screenshotting
b240432 
Some assets may take a while to load (e.g. the font); this waits for them before taking a screenshot.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 16, 2021
@thewilkybarkid
test(integration): add case for finding and reviewing a request
a4d2472 
Adds a test case for searching for a preprint that has a review request and viewing it.

The search page changes slightly when there is data, causing other reference images to be updated.

The API allows for a user to request a review more than once (which I'm not sure is expected), so this tries to make sure it's only done once. Ideally, the fixture would be set up only when it's needed.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 16, 2021
@thewilkybarkid
build(docker): mount an empty folder for the integration test results
e21dc0c 
GitHub Actions has seen some failures due to permission problems. This is caused by the mounted folder not existing, so Docker has to create it. On Linux, the permissions are based on the container, which causes a failure when the host tries to subsequently write to it. (This doesn't happen on Macs due to the Docker For Mac intermediary virtual machine.)

So, this makes sure the folder exists but is empty before mounting.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 16, 2021
@thewilkybarkid
build(make): fix removal of files in an empty directory
0817485 
e21dc0c broke when the directory was empty; this should now do nothing in those cases.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 16, 2021
@thewilkybarkid
build(docker): increase timeout when running integration tests
0db8038 
Increase the timeout for the application to start so that it matches the smoke tests; this should mitigate nondeterministic failures in CI.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 16, 2021
@thewilkybarkid
test(integration): wait for all frames to load before screenshotting
4024acf 
Expands on b240432 so that it waits for all frames on the page to load, not just the main one.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 16, 2021
@thewilkybarkid
fix: only use the content URL from Crossref if the content type is set
576e91d 
Turns out the failures I've been seeing in CI are the result having triggered a bug locally: it should not be possible to see a HTML page embedded, only a PDF. The metadata resolver was finding a content URL from Crossref (HTML) but using the content type from Google Scholar (PDF). This tries to group the content URL and type in the Crossref response together, so it uses both of them together or neither.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 24, 2021
@thewilkybarkid
test(integration): add a case for the welcome message
db89b41 
Adds a test case for checking that new visitors see a welcome message.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 24, 2021
@thewilkybarkid
test(integration): set up state using fixtures
324e09a 
Uses Playwright fixtures to capture returning-visitor state, rather than having to click the 'Get started' button every time.

This change also helps to make the tests more declarative, so there's no need for 'describe' blocks.

Refs #388
thewilkybarkid added a commit that referenced this issue Aug 24, 2021
@thewilkybarkid
test(integration): use fixture
b51bf2c 
It turns out 324e09a missed one test case.

Refs #388
thewilkybarkid added a commit that referenced this issue Oct 8, 2021
@thewilkybarkid
ci: check that the migration files are up to date
d4c18e4 
When the code is changed, we might need migration files to ensure the database has the structure that the code expects. The ORM library can generate these, but they have to be manually run and checked. This change adds a check to CI to ensure that they are updated (i.e. the ORM doesn't find any differences).

I expect CI to fail, as they are not currently up to date.

Refs #388, #395, #400
thewilkybarkid added a commit that referenced this issue Oct 11, 2021
@thewilkybarkid
fix: add and remove missing database indexes
ccde93a 
The database migrations are out of sync with the codebase:

1. The code doesn't know about some of the indexes in the migrations.
2. The code has indexes that don't exist in the migrations.
3. The code has misconfigured indexes; the ORM cannot read them but doesn't error until it tries to apply the broken SQL.

These problems caused a failure in CI as it now checks that no migrations are missing. This change removes the broken and unapplied changes and adds in code for the missing ones to allow CI to pass.

I'm not sure if any of the valid but unapplied indexes would help with performance issues, but they can be revisited later.

Refs #388, #395, #400, d4c18e4
thewilkybarkid added a commit that referenced this issue Oct 11, 2021
@thewilkybarkid
ci: don't try and diff if there's no input
bb79faa 
The Ubuntu behaves differently from my local macOS in that the reference /dev/null doesn't work if the input is empty (i.e. there are no untracked changes). Luckily GNU xargs has a switch to not run in these cases (i.e. it's possible on Ubuntu but not macOS).

Refs #388, #395, #400, 54be830
thewilkybarkid added a commit that referenced this issue Oct 12, 2021
@thewilkybarkid
build: use TypeScript for Jest tests
883f01b 
While there aren't any Jest tests yet, this change sets up ts-jest so we can write tests in TypeScript.

Refs #388, #398
@thewilkybarkid thewilkybarkid mentioned this issue Oct 12, 2021
Open
thewilkybarkid added a commit that referenced this issue Oct 22, 2021
@thewilkybarkid
fix: don't try and update the Fathom site ID if the frontend isn't there
4878faa 
If the frontend isn't available (probably not possible yet, but will be when testing the backend directly), there will be no response body. This change makes sure it's stream-like before trying to do so.

Refs #388, #398
thewilkybarkid added a commit that referenced this issue Oct 22, 2021
@thewilkybarkid
test(backend): add API docs smoke tests
9e22cd4 
Adds a simple test case that checks that the API docs returns a successful HTTP response.

As the first test, we can finally remove the --passWithNoTests switch. (Though unfortunately, it's not a unit test.)

Refs #388
thewilkybarkid added a commit that referenced this issue Oct 26, 2021
@thewilkybarkid
ci: only store integration test results if the step ran
620b214 
Use of failure() in GitHub Actions causes a step to run if any previous step fails. We want only to try and store the integration test results if that particular step fails; otherwise, there's nothing to do.

We still have to use the global failure() condition as steps don't run if something fails.

Refs #388, #395
thewilkybarkid added a commit that referenced this issue Oct 26, 2021
@thewilkybarkid
test(jest): don't collect code coverage by default
ba79ea2 
Code coverage is mostly a misleading metric, but it also slows Jest down a lot. This change stops it from always being collected.

Refs #388
thewilkybarkid added a commit that referenced this issue Oct 26, 2021
@thewilkybarkid
test(jest) allow tests to run for longer
4cb6f22 
I can't work out why Jest is timing out in CI; I'm not able to reproduce it locally. To see if it really is just being slow, this increases the timeout for a test to be 30 seconds rather than 5.

It also disables ts-jest's diagnostics, which should improve performance slightly too.

Refs ba79ea2, #388
thewilkybarkid added a commit that referenced this issue Oct 26, 2021
@thewilkybarkid
test(jest): disable log output
76f5a14 
This change disables logging to keep the test output cleaner (and ensures that the logger is actually used).

Refs #388
thewilkybarkid added a commit that referenced this issue Oct 28, 2021
@thewilkybarkid
test(backend): add a case for not finding a preprint
2c6e835 
This change adds a test case for when a preprint cannot be found.

The test case is straightforward but requires changes to be able to support it. As there's a strong coupling between the code and the ORM (e.g. through `getFields`), we can't replace the repositories with in-memory versions. Worse, some of the code requires Postgres SQL, rather than the generic type we can use across different databases.

As a result, we have to try and use an actual Postgres instance. Doing so makes the tests slower and harder to develop. (Docker solves the problem for CI, but Jest tests should be isolated enough that they can be run locally, such as in the developer's IDE.)

To try and reduce the complexity, this uses pg-mem (https://github.com/oguimbal/pg-mem), an in-memory emulation of Postgres. I've not used this before, and the readme clearly labels it as experimental, but it's worked for what I've tried so far. It has a nice rollback feature, which allows resetting the database before each test. There's probably going to be pitfuls, but it seems to be worth continuing.

I had to change the codebase to allow passing in pg-mem's customised MikroORM. Unfortunately, there isn't a clean separation between the ORM and its database connection; I'd be much happier just passing in the latter to the app, but this will have to do for now.

Refs #388, #391, #419
thewilkybarkid added a commit that referenced this issue Oct 28, 2021
@thewilkybarkid
test(backend): add a case for finding preprint information from Crossref
1e0840e 
This change adds a test case for resolving basic preprint information from the Crossref API.

It uses Nock (https://github.com/nock/nock) to prevent actual HTTP requests from being made and returns a mock response for the Crossref API.

Refs #388
thewilkybarkid added a commit that referenced this issue Nov 1, 2021
@thewilkybarkid
test(backend): add failing test cases for Zenodo failures when submit…
3fbf896 
…ting a new full review

This change adds failing test cases for Zenodo failures: the app still saves a review even when a request to Zenodo hasn't succeeded.

Refs #388, #400
thewilkybarkid added a commit that referenced this issue Nov 11, 2021
@thewilkybarkid
test(backend): add basic request test cases
b8120c2 
Refs #388, #425
thewilkybarkid added a commit that referenced this issue Nov 11, 2021
@thewilkybarkid
test(backend): add failing author request test cases
31ff753 
This change adds test cases for creating an author review which shows the app fails.

Refs #388, #425
thewilkybarkid added a commit that referenced this issue Nov 11, 2021
@thewilkybarkid
test(jest): set test path
bbc41b5 
This change stops Jest from trying to run the Backstop tests when outside of the container.

Refs #388
thewilkybarkid added a commit that referenced this issue Nov 11, 2021
@thewilkybarkid
test(jest): don't include data in the database backups
b93d436 
CI started to fail with database connection problems; I could recreate this locally using Jest's --runInBand option. For an unknown reason, it doesn't like persisting data in the beforeAll step when only using a single worker.

As a result, I've had to create these groups explicitly when needed.

Refs 31ff753, #388, #425
thewilkybarkid added a commit that referenced this issue Nov 12, 2021
@thewilkybarkid
test(integration): add case for viewing a request by an author
89646b7 
Refs #388, #425
thewilkybarkid added a commit that referenced this issue Nov 15, 2021
@thewilkybarkid
test(integration): don't try and recreate preprints
bb4d72d 
If a test fails, retries attempt to recreate the same preprint. This change adds a check to whether it needs to create it.

The main problem is that the integration tests all run against the same app, sharing state. There's more than can be done here, but this seems enough for the moment.

Refs #388, 89646b7
thewilkybarkid added a commit that referenced this issue Nov 15, 2021
@thewilkybarkid
test(integration): normalise the page when taking a locator screenshot
dfeffbe 
The page normalisation steps only run when taking a screenshot of the entire page or the current screen. This change allows them to run when taking a screenshot of a locator too.

The most important of these steps is the removal of transitions, as there have been race conditions in the Material UI click effect.

Refs #388, bb4d72d
thewilkybarkid added a commit that referenced this issue Nov 15, 2021
@thewilkybarkid
test(integration): hide all CSS animations and transitions
c12bc98 
We've continued to see race conditions on the Material UI ripple effect; it turns out that it's a CSS animation, so they need to be disabled too.

I've also updated the animation and transition normalisation to make it run extremely fast rather than not at all. This change means that JavaScript listening for an animation event would still work.

Refs #388, dfeffbe, mui/material-ui#16367 (comment)
thewilkybarkid added a commit that referenced this issue Nov 19, 2021
@thewilkybarkid
test(frontend): add cases for the rapid reviews bar chart
740b024 
This change adds test cases for the bar chart that shows for rapid reviews on a preprint. It includes a failing case too.

As the first frontend test, it uses JSDOM (https://github.com/jsdom/jsdom) and Testing Library (https://testing-library.com/). Beyond the failing test case, it also shows some design problems that make the component hard to test.

Refs #388, #409
thewilkybarkid added a commit that referenced this issue Nov 26, 2021
@thewilkybarkid
test(integration): delay screenshots slightly to avoid JS animations
a51f503 
We've seen some nondeterministic failures in CI due to animation race conditions. It seems like these animations are JavaScript based rather than CSS, and there's not a way to disable them. This change adds a 300-millisecond delay before taking the screenshot, which seems to be the default duration (https://github.com/mui-org/material-ui/blob/174b64f8a3adffb8df3c13285f4edc2437a8547a/packages/material-ui/src/internal/animate.js#L8).

Delays are never ideal, but I can't see another approach at the moment. It's also unclear whether this is enough for CI to pass, as the failure rate is far lower locally.

Refs #388, d546124, c12bc98, mui/material-ui#16367
thewilkybarkid added a commit that referenced this issue Dec 9, 2021
@thewilkybarkid
test(integration): add cases for adding reviews and requests to a com…
ed18619 
…munity

This change adds test cases for adding reviews and requests to a community, and that regular users do not see the button.

The former set currently fails as community moderators do not see the button to add reviews and requests (only admins do).

Refs #388, #429
thewilkybarkid added a commit that referenced this issue Dec 14, 2021
@thewilkybarkid
test(integration): add cases for editing a community
fa31ef1 
This change adds test cases for seeing or not the edit community button.

I've had to add a label to the button to select it, which also helps make it a little more accessible.

Refs #388, #431
thewilkybarkid added a commit that referenced this issue Dec 14, 2021
@thewilkybarkid
test(integration): add cases for joining a community
1b545a5 
This change adds test cases for joining a community. It includes a failing case for existing members who still see the button to join.

Refs #388, #431
thewilkybarkid added a commit that referenced this issue Dec 14, 2021
@thewilkybarkid
test(backend): add test cases for resolving a known DOI
835c740 
This change adds test cases for resolving a preprint with a DOI that is already known, including a failing case where the DOI used is in a different case (as it should be case insensitive).

Refs #388, #430
thewilkybarkid added a commit that referenced this issue Dec 15, 2021
@thewilkybarkid
test(backend): add failing case for duplicate requests
09e9c57 
Refs #388, #432
thewilkybarkid added a commit that referenced this issue Dec 15, 2021
@thewilkybarkid
test(backend): add failing case for duplicate requests from different…
ddb4bd0 
… personas

Refs #388, #432
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thewilkybarkid thewilkybarkid

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@thewilkybarkid