[Chasing Concept ACK]: Replacing CI tests with a python script #194
Conversation
elichai
changed the title
elichai
force-pushed
the
2020-01-tests
branch
from
b351c39
to
a4dcd24
Compare
| path = main_path.joinpath("no_std_test") | ||
| call('cargo run --verbose --release | grep -q "Verified Successfully"', path) | ||
|
|
||
| # tes cargo web |
| def call(command, cwd): | ||
| print(command) | ||
| try: | ||
| assert subprocess.check_call(command, shell=True, cwd=str(cwd)) == 0 |
There was a problem hiding this comment.
The assert is superfluous. Is it used for reader's convenience?
(https://docs.python.org/3.5/library/subprocess.html#subprocess.check_call)
There was a problem hiding this comment.
you're right. I changed the method multiple times and didn't realize this one throws and I can remove the assert.
|
Is there any reason this PR didn't get traction? A move to testing using python instead of bash may be a good move across the rust-bitcoin stack. I like Bash as much as the next bloke but as was shown by the current WASM trouble its non-trivial to write Bash scripts that do exactly what you expect them to do. |
|
Strong concept ACK moving away from bash. I think @thomaseizinger also suggested switching to full GH actions. I am in favor of it and am interested in what others of it. |
|
At least personally I find this, very very substantially less readable than ~anything in bash. bash is a scripting language designed around calling a series of programs, python...is not. I get that that's not always a common view, but bash is just commands, which we all know how to do, python is a language that not all of us actually know. |
|
I agree with @TheBlueMatt, but I also agree with @tcharding. It is much harder to read Python, but on the flip side Python is much less batshit. (Although it is pretty batshit, e.g. the behavior when you provide If anybody knew Perl in the 21st century that might be a better alternative :P. Unfortunately these two conflicting views mean that this PR probably won't be able to gain a lot of traction. |
|
Unless you are mad keen on working on it @elichai, I can add to my todo list to test all the combinations in Bash as you tried to do here. I'm not sure about the 'make the script easier to run locally to reproduce CI failures bit' though, did you still want that? |
|
I think we could probably just clean up our bash scripts and write some style guidelines; making sure we have |
|
What do we think of shellcheck? Could run that in CI on the CI scripts. Other alternatives are:
|
|
ooo shellcheck is nice. I don't have a strong opinion on any of your suggested solutions @thomaseizinger, open to follow the lead of others on this. If we stick with Bash I might have a go at hacking the script using the 'Bash std lib' that used to be part of rustup.sh. |
|
I like the idea of using shellcheck (and even enforcing it in CI). I don't really like the idea of having multiple scripts (though at some level of complexity it makes sense -- e.g. Bitcoin Core does this, and uses env vars to choose which scripts are run on which arches). And definite NACK on furtherng Github's influence on our infrastructure. |
So happy to read this :) |
Hi,
A while we realized we broke fuzztarget linking (#191)
I remember similar instances a while ago with benchmarks and other stuff.
We never run the tests we have in
secp256k1-sysbecause it was never added to the CI since the move (for fairness it's just 2 tests).On top of that we slowly add more and more features and tests, which might not work with some combinations (which can easily happen because cargo aggregate features across crates in the dependency graph)
So I decided to write a script that does the tests combinatorially, such that we test every possible combinations.
I decided to write it in python for 2 reasons, 1. Easier to do combinatorics. 2. easier to run locally to reproduce a CI failure.
Future additions:
Parsing the feature set directly from
Cargo.toml. (either with naive parsing or using a dep)