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

ci/cd: gains GH action to automate checks and tests #25

Merged
merged 12 commits into from
Aug 9, 2024

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Aug 9, 2024

I have added a ci.yml to run a series of tests and checks on this repo on every push to main.
The following checks are conducted:

  • Installs all dependencies
  • Runs unit tests (using npm run test:unit)
  • Runs integration tests (using npm run test:integration)
  • Lints the code base for style (using npm run lint)
  • Checks all JS types (using npm run check)

Note: I expect currently that the integration tests, linter and type check will all fail at this stage.
I will open separate PRs to address those failures.

Closes #23

@jdhoffa jdhoffa requested a review from MonikaFu as a code owner August 9, 2024 10:02
@jdhoffa jdhoffa removed the request for review from MonikaFu August 9, 2024 10:08
@jdhoffa
Copy link
Member Author

jdhoffa commented Aug 9, 2024

Depends on #26 and #27

Note: there are still many type check failures... I'm not entirely sure if/ how we do/ don't want to handle this.
On the one hand, implementing typescript and type safety might make our application more robust, and might make development easier in the long run, however translating all of our JavaScript modules might be arduous and not the most valuable use of our time at this stage.

I will discuss this with @MonikaFu on Monday, and we will come to a decision.

@jdhoffa
Copy link
Member Author

jdhoffa commented Aug 9, 2024

Depends on #26 and #27

Note: there are still many type check failures... I'm not entirely sure if/ how we do/ don't want to handle this. On the one hand, implementing typescript and type safety might make our application more robust, and might make development easier in the long run, however translating all of our JavaScript modules might be arduous and not the most valuable use of our time at this stage.

I will discuss this with @MonikaFu on Monday, and we will come to a decision.

Ok scratch that, it seems that in the short-term setting "checkJs": false, is the lowest friction approach to get the type checking to function (effectively limiting type checking to typescript files).

We can explore migrating our existing JS to TS later, if we think it is a valuable thing to do.

@jdhoffa
Copy link
Member Author

jdhoffa commented Aug 9, 2024

Once #26 and #27 are merged, this should be good to go

@jdhoffa jdhoffa requested review from AlexAxthelm and MonikaFu August 9, 2024 11:14
@jdhoffa
Copy link
Member Author

jdhoffa commented Aug 9, 2024

So it seems like the linter is still failing after #26 was merged.
These failures seem to be legitimate styling problems with our JS code (hey look, we are catching those now!!)

I would suggest that we merge this PR without solving those, and handle them in a separate issue, since they will likely require @MonikaFu to eventually have a look at, and she will be actively developing on a lot of those files next sprint.

So I would say this is ready for review @AlexAxthelm

Copy link
Contributor

@AlexAxthelm AlexAxthelm left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me? I've never set up JS checks before.

@jdhoffa
Copy link
Member Author

jdhoffa commented Aug 9, 2024

Sweet :-)

Will wait for @MonikaFu to give it the once over before merging

Copy link
Contributor

@MonikaFu MonikaFu left a comment

Choose a reason for hiding this comment

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

LGTM. I will need to re-write big parts of the code anyway and I agree it is better to tackle the styling then.

@jdhoffa jdhoffa merged commit 21e425f into main Aug 9, 2024
4 of 5 checks passed
@jdhoffa jdhoffa deleted the ci-cd_automate_checks branch August 9, 2024 14:12
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.

ci/cd: add GH action to automate running of tests and checks
3 participants