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

Run test suite in GH Actions #195

Closed
wants to merge 202 commits into from

Conversation

harlantwood
Copy link

@harlantwood harlantwood commented Feb 22, 2022

  • delete old circle and travis stuff

Runs in Ubuntu and Mac. Would run in Windows too, but I don't think nix works there.

I also came across these lines in circle and travis configs (circle has not run for 2 years, travis I'm not sure) -- maybe these are useful ways to run the tests? @pospi LMK if you'd like different test commands, or if you want test in GH actions at all.

nix-shell --run 'npm i -g pnpm' && nix-shell --run 'npm run dht:sim2h &'
nix-shell --run 'pnpm install --network-concurrency 1 && npm run build && npm run test:integration:test'
nix-shell --run 'pnpm install && npm run build && npm run test:integration:test'

pospi and others added 2 commits February 22, 2022 20:00
…w HeaderHash in preparation for indexing logic changes

DnaAddressable now must be directly storable as an Entry in order for fully-qualified IDs to be encoded in the DHT. This means conversion traits to/from hdk::prelude::Entry must be created, and the inner type of DnaAddressable cannot be ambiguous since this would make implementing such conversions impossible. This makes sense and is a good thing, because it enforces that only tuples of (DnaHash, EntryHash) can be used as identifiers. Implementors should not be linking to revisions but rather records.
@harlantwood
Copy link
Author

Update: looks like travis last ran 2 years ago as well as Circle. Links:

https://app.circleci.com/pipelines/github/holo-rea/holo-rea
https://travis-ci.org/github/holo-rea/holo-rea

…fully qualified address

necessary changes to related trait bounds to support this
@pospi
Copy link
Member

pospi commented Feb 23, 2022

@Connoropolous actually just submitted a PR for github actions, and those are enabled now to generate builds.

Whether we want the test suite to run as part of that process is another matter, possibly an evolving one. At present still maybe 40-50% of the test suite is failing, mostly just because they haven't yet been updated for RSM (see #174). So that's why I haven't yet re-enabled Travis.

There might be a worthwhile interim step where we run tests on one CI system and builds on another. Tests should be run for any change on sprout, but builds only get run if you push a version tag. It's possibly also worthwhile keeping them as parallel things on different infrastructure because it means, well, parallelisation.

Eventually though yes I think it's best if everything gets run in GH actions and the build doesn't get kicked off if the tests fail!

Oh, and a note on the test suite- I didn't have many Rust tests in the beginning so was only running Tryorama tests in JS. Eventually I think it would be best to simply run npm run test, which first runs unit tests with cargo test before triggering the integration test suite in the test/ subdirectory. Rust tests probably need a QA and some polish before doing that, and the stuff in test/core-architecture should be being done with cargo rather than nodejs & tape (see #43).

Does that help? Opinions & input very welcome (:

@Connoropolous
Copy link
Contributor

I still haven't sent Pospi the cachic secret, that we can add as a secret to the repository, so I will do that now.

@Connoropolous
Copy link
Contributor

I also just approved the CI runs for this PR, since Harlan and this fork are 'new contributors'

@Connoropolous
Copy link
Contributor

myself I've never yet tried running the test suite, so I can't comment like Pospi can

.cargo/registry/cache/
.cargo/git/db/
target/
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

this repo has the rather anomalous behavior of not checking the Cargo.lock file into git, which as I understand it has its reasons, but from my experience has had some real downsides

Copy link
Contributor

Choose a reason for hiding this comment

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

in my other one, I use Cargo.toml, while realizing that that's not best practice

Copy link
Author

Choose a reason for hiding this comment

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

this repo has the rather anomalous behavior of not checking the Cargo.lock file into git, which as I understand it has its reasons, but from my experience has had some real downsides

Hmm I think this is actually best practice for libraries (but not binaries) — https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

Copy link
Author

Choose a reason for hiding this comment

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

in my other one, I use Cargo.toml, while realizing that that's not best practice

Thanks, should update

Copy link
Contributor

Choose a reason for hiding this comment

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

That kinda makes sense. The zomes themselves are more like binaries, while the things in lib are libraries, so 🤷 I'm not sure

Copy link
Author

Choose a reason for hiding this comment

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

That kinda makes sense. The zomes themselves are more like binaries, while the things in lib are libraries, so 🤷 I'm not sure

More nuanced suggestion here: https://stackoverflow.com/a/63727523

@harlantwood
Copy link
Author

harlantwood commented Feb 24, 2022 via email

@harlantwood
Copy link
Author

Tests should be run for any change on sprout, but builds only get run if you push a version tag.

That’s exactly the behavior of this PR was merged. And they would run in parallel if you pushed a tag.

Eventually though yes I think it's best if everything gets run in GH actions and the build doesn't get kicked off if the tests fail!

Sure, we could chain the builds conditionally later.

@harlantwood
Copy link
Author

I still haven't sent Pospi the cachic secret, that we can add as a secret to the repository, so I will do that now.

Ah cool. Still the run was about 5 min, must have been found in the holochain-ci cache.

@harlantwood
Copy link
Author

@pospi overall I’d suggest you just leave this open until you’re ready to run tests in CI, happy to help more at that time if desired.

@Connoropolous
Copy link
Contributor

"have you considered moving to sweetest instead of upgrading
tryorama? It’s pretty great to have your tests of rust lib in rust. It’s
slow (unless you’re on an M1 mac) but worth it in my experience so far."
I am speaking out of turn, but I do know that the tests actually test the graphql part of the stack as well, so it's not useful to switch off of tryorama

Comment on lines +44 to +50
- name: Set up nix
uses: cachix/install-nix-action@v16
with:
nix_path: nixpkgs=channel:nixos-21.05
extra_nix_config: |
substituters = https://cache.nixos.org https://cache.holo.host https://ci-builds.cachix.org https://holochain-ci.cachix.org
trusted-public-keys = cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY= cache.holo.host-1:lNXIXtJgS9Iuw4Cu6X0HINLu9sTfcjEntnrgwMQIMcE= cache.holo.host-2:ZJCkX3AUYZ8soxTLfTb60g+F3MkWD7hkH9y8CgqwhDQ= ci-builds.cachix.org-1:fxB0+h/MMlCpXf6hFsQM31YpHbaQoRmcNPNHwDUkXA4= holochain-ci.cachix.org-1:5IUSkZc0aoRS53rfkvH9Kid40NpyjwCMCzwRTXy+QN8=
Copy link
Contributor

Choose a reason for hiding this comment

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

we should consolidate between this way of defining these, and the nix.conf way that is currently at play. I have zero attachment to which way it goes

Copy link
Contributor

Choose a reason for hiding this comment

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

I am referring to this file, and the command in the Release CI workflow that references it
https://github.com/holo-rea/holo-rea/blob/sprout/.github/nix.conf

pospi added 18 commits May 4, 2022 13:31
…into LinkTag

fixes Unit CRUD tests in h-REA#249 by allowing string IDs to be queried from the DHT again, after PathEntry changes made this impossible using Path construct
…g issues.

All index updates & RPC calls now uniformly treated as non-critical and logged for output to assist in debugging.
@pospi
Copy link
Member

pospi commented May 16, 2022

Just noting here that all integration tests are passing again, so we can probably go ahead with this.

I'd also like to see checks configured so that feature branches can't be merged into sprout without the integration test suite passing 🙂

@Connoropolous Connoropolous changed the title [ON HOLD] Run test suite in GH Actions Run test suite in GH Actions May 19, 2022
@Connoropolous
Copy link
Contributor

closing in favor of #267 since the git histories became diverged in a weird way I had trouble with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-writing Issues related to unit or integration tests
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants