Skip to content

CI: run unit tests on multiple Nodejs versions - #220

Merged
kravets-levko merged 3 commits into
mainfrom
ci-multi-nodejs-versions
Jan 24, 2024
Merged

CI: run unit tests on multiple Nodejs versions#220
kravets-levko merged 3 commits into
mainfrom
ci-multi-nodejs-versions

Conversation

@kravets-levko

@kravets-levkokravets-levko commented Jan 22, 2024

Copy link
Copy Markdown
Contributor

Configure CI to run unit tests on different Nodejs versions.

E2e are a somewhat time and resource consuming, so for now they will be executed only once.

It should help with preventing compatibility issues in future (see #219)

Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
@kravets-levko
kravets-levkoforce-pushed the ci-multi-nodejs-versions branch from d4bcd7e to a0181adCompareJanuary 23, 2024 15:02
@kravets-levko
kravets-levkoforce-pushed the ci-multi-nodejs-versions branch from a0181ad to 27898ecCompareJanuary 23, 2024 15:07
@kravets-levko
kravets-levkoforce-pushed the ci-multi-nodejs-versions branch from 27898ec to d09e99fCompareJanuary 23, 2024 15:10
@kravets-levko
kravets-levkoforce-pushed the ci-multi-nodejs-versions branch from d09e99f to 4b87498CompareJanuary 23, 2024 15:15
@kravets-levko
kravets-levkoforce-pushed the ci-multi-nodejs-versions branch from 4b87498 to 54baaf4CompareJanuary 23, 2024 15:25
@kravets-levko
kravets-levkoforce-pushed the ci-multi-nodejs-versions branch from 54baaf4 to 5ceecdbCompareJanuary 23, 2024 15:51
@kravets-levko
kravets-levkoforce-pushed the ci-multi-nodejs-versions branch from 5ceecdb to 0624c06CompareJanuary 23, 2024 15:55
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a4e4aa7) 90.41% compared to head (1862ea6) 90.41%.

Additional details and impacted files
@@ Coverage Diff @@## main #220 +/- ##
=======================================
Coverage 90.41% 90.41% =======================================
Files 61 61 Lines 1408 1408 Branches 232 232 =======================================
Hits 1273 1273 Misses 84 84 Partials 51 51 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -32,30 +32,39 @@ jobs:

unit-test:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I recommend separating the unit-tests/linting from the E2E tests. Since the unit tests do not depend on secrets, you can have a one-click approval to run them on public PRs if you separate them into their own Github Action file. That way you can see if a public PR at least passes unit tests before going through the process of making a branch in the local fork in order to run the E2E tests. Another reason to separate them out is that, because they are much cheaper to run, you can have different trigger conditions/exclusions.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently unit and e2e tests are running as a separate jobs, but in single workflow. It should stay this way in order to properly collect coverage data (each job emits own coverage report; separate job waits for test jobs, then retrieves all coverage files and sends them to codecov)

@kravets-levkokravets-levkoJan 23, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

+ unit tests indirectly depend on secrets (coverage job needs a Codecov token stored as secret)

E2E_ACCESS_TOKEN: ${{ secrets.DATABRICKS_TOKEN }}
E2E_TABLE_SUFFIX: ${{github.sha}}
cache-name: cache-node-modules
NYC_REPORT_DIR: coverage_e2e

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For my knowledge, where / how do these get used? It might be a pattern I want to pull into dbt-databricks.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I use this env here to have unique filenames for coverage reports from different jobs (e.g. coverage_e2e, coverage_unit_node16, etc.), and because that names are used in many places across the workflow file. Those files are later collected by another job which then sends them to Codecov. We cannot send them one by one, because in this case Codecov won't merge those coverage reports and just use the last submitted one

@benc-dbbenc-db left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider splitting out E2E from the cheaper actions that do not rely on secrets. Otherwise, LGTM

@susodapopsusodapop left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stamp

@kravets-levko
kravets-levko merged commit 086f5bb into mainJan 24, 2024
@kravets-levko
kravets-levko deleted the ci-multi-nodejs-versions branch January 24, 2024 18:01
Sign up for freeto 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.

4 participants

@kravets-levko@codecov-commenter@susodapop@benc-db