cytoscape / cytoscape-explore

Network visualization webapp.
MIT License
12 stars 4 forks source link

Fix CI #141

Closed maxkfranz closed 2 years ago

maxkfranz commented 2 years ago

General information

This PR fixes CI so that everything is green.

Associated issues:

Checklist

Author:

Reviewers:

Notes

Broad summary (details in commits)

maxkfranz commented 2 years ago

@d2fong, a good test to verify that this PR is sufficient would be to merge in fix/no-red into your PR's branch. If your PR is green after that, then it's probably good.

d2fong commented 2 years ago

One of the tests fails for me still:

  1) History (Snapshots)
       creates thumbnails:
     Error: Timeout of 15000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/dylan/src/idekerlab/cytoscape-explore/test/history-test.js)
      at listOnTimeout (node:internal/timers:557:17)
      at processTimers (node:internal/timers:500:7)
d2fong commented 2 years ago

But I ran it again and it passes. Maybe the time limit should be increased?

maxkfranz commented 2 years ago

15s is too long. If it takes that long, the user is going to be left hanging. Ultimately still a test failure in my opinion. It’s hard to get puppeteer setups just right

We could comment out that test and make an issue for the problem if we want to move forward quickly

On Mar 3, 2022, at 19:21, Dylan Fong @.***> wrote:

 But I ran it again and it passes. Maybe the time limit should be increased?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

maxkfranz commented 2 years ago

The test is commented out in 608f827. #145 needs to be fixed before dockerised CI instances will run correctly

maxkfranz commented 2 years ago

@d2fong, do you want to put the patch for #145 in this PR? It looks like GH Actions was able to reproduce the checksum error affecting docker: https://github.com/cytoscape/cytoscape-explore/runs/5452877547?check_suite_focus=true

d2fong commented 2 years ago

Ok I pushed a fix. We were working on refactoring the monorepo stuff so the dependency broke. Sorry.

d2fong commented 2 years ago

@keiono, how would I update the ndex-client dependency using the new monorepo?

d2fong commented 2 years ago

@maxkfranz I think there is an issue with installing a git repo branch as a dependency when running the dockerized CI instance. Is this by design or is it ok to use branches from a git repo as a dependency?

maxkfranz commented 2 years ago

I think you can use a git repo as a dependency, but the package manager is strict with the lock files now (for security etc.). It looks like there's a hash check that's failing. So, we need the hash to be the same as what's installed when npm install was used to add to package.json.

How we make the hash consistent is up to you guys. Here are some possibilities:

(1) Use a stable branch in your repo that's not changing. Every time you change it, you have to update the lock file here in CE. I'm not crazy about this option, since it's easy to get things out of sync and the failures can be non-obvious.

(2) Point to a specific commit in the git address instead of a branch.

(3) Publish to npm so version numbers can be used. Maybe a beta version like 1.0.0-beta1?

Maybe you've got a better/simpler suggestion to get the hash right than these three? Whatever works

d2fong commented 2 years ago

I think the issue might be related to docker and git not having the right permissions.

npm ERR! /usr/bin/git ls-remote -h -t ssh://git@github.com/ndexbio/ndex-js-client.git
npm ERR! 
npm ERR! No user exists for uid 1001
npm ERR! fatal: Could not read from remote repository.
npm ERR! 
npm ERR! Please make sure you have the correct access rights
npm ERR! and the repository exists.
npm ERR! 
npm ERR! exited with error code: 12[8](https://github.com/cytoscape/cytoscape-explore/runs/5453467076?check_suite_focus=true#step:6:8)
maxkfranz commented 2 years ago

Could we use a public https address instead of a private ssh one? Is ssh needed if we're only cloning?

d2fong commented 2 years ago

Thats weird because I specified it as git+https://github.com/ndexbio/ndex-js-client.git#develop

d2fong commented 2 years ago

Ok Kei and Jing updated the proper version/monorepo

maxkfranz commented 2 years ago

@jingjingbic, @d2fong, @chrtannus -- you can merge dev into your branches for your PRs now to help make your PRs green

chrtannus commented 2 years ago

Great! I've just merged dev into feature/recent-networks and the PR and it's all green now.