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

chore(api): Fix yarn test script #429

Merged
merged 5 commits into from
Jun 3, 2021
Merged

chore(api): Fix yarn test script #429

merged 5 commits into from
Jun 3, 2021

Conversation

victorges
Copy link
Member

@victorges victorges commented Jun 3, 2021

What does this pull request do?

This mainly fixes the yarn test command on my machine.

Took quite some while to find the real issue, but in the end it was basically this same problem:
TL;DR: the argument file sent to jest is not actually a path but actually a regex matched
against the absolute file path.

Since I have all my cloned repositories under a folder like ~/workspace/src/, basically all files
inside the root folder where jest looks on were matching the filtered regex. This meant that all
test files on dist/ folder as well as the test.js in the root of the api project were being
executed as if they were tests in the project.

Specific updates

Fixed:

  • the dist/ part by ignroring .test. files on babel which I think makes sense nevertheless;
  • and the test.js part by changing the jest command to consider only the local src folder
    with src => ${PWD}/src. This should also work on Windows as per jest docs.

Tried several other options for jest like using the --roots, --rootDir and --runTestsByPath,
but all of those make the tests run really slow for some weird reason, like the actual requests
made to the test server took 100s of ms. I tried checking some configs like the DB endpoints
and fallback server but they were all fine, so I had not enough context to go further efficiently.

Also included in this change but unrelated to the main goal:

  • Created a coverage script in the api which calls the test command with the additional
    --coverage flag, to make the root project yarn coverage to start working again.
  • Fixed what seemed like a typo in the swagger schema inverting descriptions of 2 tags.

How did you test each of these updates

Ran the related yarn scripts like install, prepare, test and coverage on api and root
projects top ensure everything was fine.

Does this pull request close any open issues?

No

Checklist

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

This was one of the ways of fixing the jest issue,
to avoid having test files under the dist package.

It did not fix it completely because we still have
a `test.js` file on the root of the package (lmk if
we should remove it too)

It also doesn't fix the root cause of the issue,
which I found later.
This is what really fixed the test run in the end.
For details: jestjs/jest#8226
Simply calls the regular test with an additional arg.
This makes the root `yarn coverage` start working again,
since it looks for a script with that exact name in the
internal packages.
This is unrelated to the other changes but
really small so I felt I could just include
it here.

Just what seems like a typo having the tags
with inverted descriptions.
@vercel
Copy link

vercel bot commented Jun 3, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/livepeerorg/livepeer-com/3kNAx95EMBbYEbewTWrGnRzAfwRW
✅ Preview: https://livepeer-com-git-chore-fix-jest-cmd-livepeerorg.vercel.app

@victorges
Copy link
Member Author

victorges commented Jun 3, 2021

✅ Will investigate why the build broke on github pipeline.

I probably removed it in some of my tests and actually
forgot to undo it later.

This is was what was breaking the build since it tried
to really run the API and didn't manage to reach the
postgresql running locally.
description: User Endpoints
- name: api-token
description: Api Token Endpoints
Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, nice catch

Copy link
Contributor

@iameli iameli left a comment

Choose a reason for hiding this comment

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

LGTM!

@victorges victorges merged commit ef1bb32 into master Jun 3, 2021
@victorges victorges deleted the chore/fix-jest-cmd branch June 3, 2021 21:39
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.

2 participants