Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Add basic sharness tests #504

Merged
merged 12 commits into from
Oct 16, 2016
Merged

Add basic sharness tests #504

merged 12 commits into from
Oct 16, 2016

Conversation

chriscool
Copy link
Contributor

This is to start testing js-ipfs in the same way as go-ipfs.
See issue #412 (Sharness tests).

@jbenet jbenet added the status/deferred Conscious decision to pause or backlog label Sep 22, 2016
@daviddias
Copy link
Member

\o/ Thank you @chriscool :D

Do all of these run for you? Found any issues? Can I get a status of this endeavour?

I know you have been trying to reach me through IRC, unfortunately, with DEVCON2 I wasn't the most available. I'll have good wifi when the 29th of September comes.

@chriscool
Copy link
Contributor Author

There are a few that don't pass. For example I think the first to fail is the 5st or the 6st because jsipfs version --all doesn't work properly.
If you want I can change the last commit to use test_expect_failure for those that don't pass.
But I think it can still be interesting to commit this as it makes it possible for other developers and CI tools to try to run the tests and see what fails.

The Makefile also implements make help that provides a bit of help to run the tests.

@dignifiedquire
Copy link
Member

I think it would be good to leave the failing as failing for now so we can make a list and check things off :)

@daviddias
Copy link
Member

Yeah, add all the tests @chriscool, if they fail, the better, more stuff for us to fix :D

@chriscool idealy, we want:

  • A PR that sets up the sharding tests (the ones that pass already) and then
  • PR for each of the subcommands, with all the tests, highlighting the ones that fail

@chriscool
Copy link
Contributor Author

@diasdavid ok, I removed failing tests from t0010, so now everything should pass and you can merge this.
I am preparing other PRs, some that add passing tests, some that add failing tests.
I will send them when this PR will be merged, as they are based on it.

@chriscool
Copy link
Contributor Author

chriscool commented Oct 3, 2016 •
edited
Loading

By the way, tests should either be run using make or make deps should be used before running the tests.

This is because a symbolic link from 'js-ipfs/test/bin/ipfs' to 'js-ipfs/src/cli/bin.js' is used to make it possible for the tests to run without having to change them much.

The path to 'js-ipfs/test/bin' is added into PATH by test-lib.sh which is sourced by all the test scripts.

@chriscool
Copy link
Contributor Author

This last commit adds a check and improves the error message.

daviddias
daviddias reviewed Oct 3, 2016
View reviewed changes
test/ipfs-test-lib.sh
@@ -0,0 +1,107 @@
# Generic test functions for go-ipfs
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can start calling it the IPFS cli :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Year, sure I will change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to: # Generic test functions for ipfs cli tests

daviddias
daviddias reviewed Oct 3, 2016
View reviewed changes
test/sharness/Makefile
@which curl >/dev/null || (echo "Please install curl!" && false)

.PHONY: all help clean clean-test-results $(T) aggregate deps sharness

Copy link
Member

Choose a reason for hiding this comment

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

can we bring the Makefile to the root, so we can run it on CI by adding a line with make to:

https://github.com/ipfs/js-ipfs/blob/master/.travis.yml#L14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In go-ipfs there is also a Makefile at the root, but it is different.
You use make test at the root and make when you are in test/sharness.
I can do something like that but it could go into another PR I think as it is related to the whole project.

Copy link
Member

@daviddias daviddias Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@chriscool what I'm looking for is to be able to add these sharness tests to CI before this merge. Can we get that added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, with the last commit I added you can just run make test at the root.
I will add that to the '.travis.yml'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added that and it works.

daviddias
daviddias reviewed Oct 3, 2016
View reviewed changes
test/sharness/lib/test-lib-hashes.sh
HASH_WELCOME_DOCS="QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG"
HASH_GATEWAY_ASSETS="QmXB7PLRWH6bCiwrGh2MrBBjNkLv3mY3JdYXCikYZSwLED"
HASH_HELP_PAGE="QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7"
HASH_EMPTY_DIR="QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn"
Copy link
Member

Choose a reason for hiding this comment

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

Apparently, these change often (//cc @lgierth) how does go-ipfs handle it? Changing it manually everytime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't change so often, so I think manually is fine for now.
Last time they changed was in January.

Copy link
Member

Choose a reason for hiding this comment

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

We update them manually here every time we update them in go-ipfs

@daviddias
Copy link
Member

daviddias commented Oct 3, 2016 •
edited by chriscool
Loading

\o/ woot!

There is a lot of bash magic, but they do work for me, so awesome! :D

Once we have CI running these, I'm happy to merge. Then we can see all the remaining PR's for each subcommand coming in :)

Super excited about this, getting the same level of robustness as the Go implementation 🌟

@chriscool
Copy link
Contributor Author

@diasdavid just pushed with the following changes:

  • added a Makefile at the root as in the last commit,
  • changed the first line of test/ipfs-test-lib.sh

@whyrusleeping
Copy link
Member

This all LGTM. Its just the base sharness setup, same as in go-ipfs

@chriscool
Copy link
Contributor Author

@diasdavid could you take a look at merging this?

@daviddias daviddias merged commit 7515aec into ipfs:master Oct 16, 2016
@daviddias
Copy link
Member

@chriscool done, thank you :)

Can you start opening the PR for each of the subcommands now? If possible, listing which tests fail, so that we can fix them :)

@chriscool
Copy link
Contributor Author

@diasdavid ok I will do that now.

MicrowaveDev pushed a commit to galtproject/js-ipfs that referenced this pull request May 22, 2020
@hugomrdias @alanshaw
fix: reword resolve test with async/await (ipfs#504)
3f7410a
This was referenced Apr 7, 2022
Open
Open
Open
@awaisab172 awaisab172 mentioned this pull request Jun 21, 2023
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@daviddias daviddias daviddias left review comments

@whyrusleeping whyrusleeping whyrusleeping left review comments

Assignees
No one assigned
Labels
status/deferred Conscious decision to pause or backlog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chriscool @daviddias @dignifiedquire @whyrusleeping @jbenet