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: Test matrix #1862

Closed
wants to merge 11 commits into from
Closed

Conversation

nasdf
Copy link
Member

@nasdf nasdf commented Sep 8, 2023

Relevant issue(s)

N/A

Description

This PR has been merged into #1839 due to flaky CLI tests causing failures.

This merges all test variants into a single test matrix and includes a few other updates:

  • Merge code-test-coverage into run-tests workflow
  • Batch upload code coverage from all jobs after a successful run
  • Replace go-acc with go 1.20 aggregate test coverage
  • Run change detector as a matrix job ci: Parallelize change detector #1871

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

CI

Specify the platform(s) on which this was tested:

  • MacOS

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.09% ⚠️

Comparison is base (b8567c2) 70.26% compared to head (3f4c940) 70.17%.

❗ Current head 3f4c940 differs from pull request most recent head d9be82d. Consider uploading reports for the commit d9be82d to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1862      +/-   ##
===========================================
- Coverage    70.26%   70.17%   -0.09%     
===========================================
  Files          232      225       -7     
  Lines        24192    23987     -205     
===========================================
- Hits         16998    16832     -166     
+ Misses        6029     5996      -33     
+ Partials      1165     1159       -6     
Flag Coverage Δ
all-tests 70.17% <ø> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 23 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8567c2...d9be82d. Read the comment docs.


runs-on: ubuntu-latest

strategy:
matrix:
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: 😍 😍 😍 I did not know we could do this. And I really want it. Thank you very much for opening this draft.

client-type: [go, http]
database-type: [badger-file, badger-memory]
mutation-type: [gql, collection-named, collection-save]
tests: [names, lens, cli]
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I would remove this line, and instead all items can just test ./....

database-type: [badger-file, badger-memory]
mutation-type: [gql, collection-named, collection-save]
tests: [names, lens, cli]

Copy link
Contributor

@AndrewSisley AndrewSisley Sep 9, 2023

Choose a reason for hiding this comment

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

thought: Once we remove the -p 1 limitation, we can include the change detector in this file using an include, I think the below might work:

 include:
      - client-type: go
        database-type: badger-memory
        mutation-type: collection-save
        detect-changes: true

Copy link
Member Author

Choose a reason for hiding this comment

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

Added both the change detector and non change detector to the include so both will now run.

database-type: [badger-file, badger-memory]
mutation-type: [gql, collection-named, collection-save]
tests: [names, lens, cli]

Copy link
Contributor

Choose a reason for hiding this comment

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

thought: I think we should also include the codecov stuff in this file, and delete the codecov workflow. I don't want to mark this as a todo though, as I'm still playing around with it and it can be done after easily enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to try and get that working.

@@ -20,10 +20,24 @@ on:

jobs:
run-tests:
name: Run tests job
name: Run tests job matrix

runs-on: ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: We could also include this in the matrix at somepoint if we want (I would like this).


runs-on: ubuntu-latest

strategy:
matrix:
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: This will increase the visibility of any test flakiness. This could be extremely annoying, but will force us to sort out flakiness.

…rkflow. replace Makefile actions with standard actions.
go install gotest.tools/gotestsum@latest
go install github.com/ory/go-acc@latest

- name: Build Lens dependencies
Copy link
Contributor

@AndrewSisley AndrewSisley Sep 11, 2023

Choose a reason for hiding this comment

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

praise: This is really nice that we only have to do this once :)

EDIT: I mis-read :)

go install gotest.tools/gotestsum@latest
go install github.com/ory/go-acc@latest

- name: Build Lens dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: However, I think it should probably be done via a make command, instead of defined directly in the github action file

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

run: gotestsum ./... --format testname -- -race -shuffle=on -timeout 300s

- name: Generate test coverage
run: go-acc ./... --output=coverage.txt --covermode=atomic -- -failfast
Copy link
Contributor

@AndrewSisley AndrewSisley Sep 11, 2023

Choose a reason for hiding this comment

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

todo/question: This looks like it is running the tests twice, once without code coverage with our test params (race/shuffle/etc), and once with code coverage but without the params? Suggest just running the tests once, and using a make command (the make command should also build the wasm binaries, just like it builds the Go ones).

Copy link
Member Author

Choose a reason for hiding this comment

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

I converted coverage to the new go 1.20 aggregate mode and it now only runs tests once.

@nasdf nasdf added area/testing Related to any test or testing suite ci/build This is issue is about the build or CI system, and the administration of it. labels Sep 12, 2023
@nasdf nasdf mentioned this pull request Sep 20, 2023
11 tasks
@nasdf nasdf closed this in #1839 Oct 2, 2023
@nasdf nasdf closed this in 4c3df48 Oct 2, 2023
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Closes sourcenetwork#1472
Closes sourcenetwork#1507
Closes sourcenetwork#1860 

## Description

This is a follow up to sourcenetwork#1776

This PR adds a CLI implementation that implements the client.DB
interface and runs through the existing integration test suite.

- [x] Merge existing server config code
- [x] Refactor CLI to use new HTTP client
- [x] Remove `net/api` package
- [x] Remove `api/http` package
- [x] Lens tests are timing out in CI: fixed sourcenetwork#1862
- [x] Code coverage is incorrectly reporting: fixed sourcenetwork#1861
- [x] Flaky test causing failures: fixed sourcenetwork#1912

Renamed Commands:
- `peerid` to `client peer info`
- `client p2pcollection` to `client p2p collection`
- `client replicator` to `client p2p replicator`
- `client schema list` to `client collection describe`

Removed Commands:
- `block get`
- `ping`
- `rpc`

Added Commands:
- `client collection create`
- `client collection delete`
- `client collection get`
- `client collection keys`
- `client collection update`
- `client tx create`
- `client tx discard`
- `client tx commit`
- `client schema migration up`
- `client schema migration down`
- `client schema migration reload`

**Notes for reviewers**:
- `.github` changes are merged from sourcenetwork#1871
- `Makefile` most of these changes are also from sourcenetwork#1871
- `docs/cli` ignore these changes, it will be updated next release
- sorry for all of the merge commits, I am working on learning rebase
flow

## Tasks

- [x] I made sure the code is well commented, particularly
hard-to-understand areas.
- [x] I made sure the repository-held documentation is changed
accordingly.
- [x] I made sure the pull request title adheres to the conventional
commit style (the subset used in the project can be found in
[tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)).
- [x] I made sure to discuss its limitations such as threats to
validity, vulnerability to mistake and misuse, robustness to
invalidation of assumptions, resource requirements, ...

## How has this been tested?

`make test`

Specify the platform(s) on which this was tested:
- MacOS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Related to any test or testing suite ci/build This is issue is about the build or CI system, and the administration of it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants