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

fix(ci): fix sanity check and run for PRs #38

Merged
merged 2 commits into from Jul 2, 2021
Merged

fix(ci): fix sanity check and run for PRs #38

merged 2 commits into from Jul 2, 2021

Conversation

wilsonianb
Copy link
Contributor

@wilsonianb wilsonianb commented Jun 30, 2021

Changes proposed in this pull request

  • Build accounts service before running connector tests.
  • Run sanity check for pull requests (to avoid unexpected failures after merging to main).

Context

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass

Build accounts service before running connector tests.
@wilsonianb wilsonianb marked this pull request as ready for review June 30, 2021 21:35
Copy link
Collaborator

@cairin cairin left a comment

Choose a reason for hiding this comment

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

Why do we need to build the accounts service before running tests in the connector?

@sentientwaffle
Copy link
Contributor

sentientwaffle commented Jul 1, 2021

I think the packages' package.jsons need a prepare script to build. That way when the connector "installs" the accounts service, it will make sure its compiled.

@wilsonianb
Copy link
Contributor Author

Will the prepare script help with our zero-install setup?
I tried it, but running yarn install --immutable --immutable-cache doesn't appear to build the accounts service:
wilsonianb@9991ec5

@sentientwaffle
Copy link
Contributor

Interesting. It might be due to the immutable/immutable-cache... I guess the explicit building of accounts is necessary after all.

@wilsonianb
Copy link
Contributor Author

I'm seeing if jest can be configured to build accounts for us with something like moduleNameMapper
https://medium.com/ah-technology/a-guide-through-the-wild-wild-west-of-setting-up-a-mono-repo-part-2-adding-jest-with-a-breeze-16e08596f0de

@wilsonianb
Copy link
Contributor Author

I haven't been able to get ts-jest to compile accounts when running connector tests.
There's a related discussion here: kulshekhar/ts-jest#1648
I vote we keep manually building for now but keep an eye out for a better ts-jest solution.

@wilsonianb wilsonianb requested a review from cairin July 2, 2021 16:17
@wilsonianb wilsonianb merged commit 2035c47 into main Jul 2, 2021
@wilsonianb wilsonianb deleted the bw-fix-main branch July 2, 2021 21:59
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.

None yet

3 participants