Bashtub Tests Working #4586
Bashtub Tests Working #4586
Conversation
cmd/util/fatal.go
Outdated
| cmd.SetOut(os.Stdout) | ||
| cmd.Println() |
There was a problem hiding this comment.
I don't think setting the writer here is necessary as we are already doing that in cli/root
Lines 136 to 137 in 48dc1a7
|
|
||
| var bErr bacerrors.Error | ||
| if errors.As(err, &bErr) { | ||
| // Print error message | ||
| cmd.PrintErrln(output.RedStr("Error: ") + bErr.Error()) | ||
| cmd.Println(output.RedStr("Error: ") + bErr.Error()) |
There was a problem hiding this comment.
why are we switching from stderr to stdout for error messages? It doesn't sound the right thing to do
There was a problem hiding this comment.
As discussed reverting it.
| subject bacalhau config set node.network.type nats | ||
| subject ${BACALHAU} config set orchestrator.auth.token kerfuffle | ||
| create_node compute | ||
| # If this returns successfully, the node started and authenticated. |
There was a problem hiding this comment.
I don't think this should pass. We are setting orchestrator.auth.token which secures the orchestrator node, but not setting compute.auth.token. This is making me question if we are actually validating all the tests properly or not
test/update.sh
Outdated
| # testcase_default_config_has_updates_enabled() { | ||
| # subject bacalhau config list --output=csv | ||
| # assert_equal 0 $status | ||
| # assert_not_equal $(echo "$stdout" | grep 'update.checkfrequency' | cut -d, -f2) '0' | ||
| # assert_not_equal $(echo "$stdout" | grep 'update.checkfrequency' | cut -d, -f2) '' | ||
| # } |
There was a problem hiding this comment.
you shouldn't disable this, but instead check for UpdateConfig.Interval. Also I hate that we have to remember to replace bacalhau with ${BACALHAU} as we will keep forgetting as you have fixed in many places and here as well. Can't we set an alias before running the tests?
There was a problem hiding this comment.
This is mainly done for local development. Because the local environment in a developers laptop comes into the play. We can come up with an alias, but in that scenario it would be the case that we remember to use that alias.
There was a problem hiding this comment.
by alias I mean directing bacalhau to the development binary and not using a different term. As in alias bacalhau=path/to/built/binary
| # #!bin/bashtub | ||
|
|
||
| source bin/bacalhau.sh | ||
| # source bin/bacalhau.sh | ||
|
|
||
| setup() { | ||
| { | ||
| bacalhau config set auth.methods "{\"Method\": \"shared_secret\", \"Policy\": {\"Type\": \"ask\", \"PolicyPath\": \"$ROOT/pkg/authn/ask/ask_ns_secret.rego\"}}" | ||
| bacalhau config set auth.accesspolicypath "$ROOT/pkg/authz/policies/policy_ns_anon.rego" | ||
| } >/dev/null | ||
| # setup() { | ||
| # { | ||
| # bacalhau config set auth.methods "{\"Method\": \"shared_secret\", \"Policy\": {\"Type\": \"ask\", \"PolicyPath\": \"$ROOT/pkg/authn/ask/ask_ns_secret.rego\"}}" | ||
| # bacalhau config set auth.accesspolicypath "$ROOT/pkg/authz/policies/policy_ns_anon.rego" | ||
| # } >/dev/null | ||
|
|
||
| subject 'bacalhau config list | grep auth.methods' | ||
| assert_match 'shared_secret' $stdout | ||
| # subject 'bacalhau config list | grep auth.methods' | ||
| # assert_match 'shared_secret' $stdout | ||
|
|
There was a problem hiding this comment.
I don't think we should skip these tests as they might be indicating an issue with 1.5. Lets work together to debug this
|
What is the status of this PR? |
We can merge this. I have created a separate issue #4609 for auth tests. I need to investigate it more. The main problem is that our framework creates logical separation between tests and not within tests. So for example before every test we would create a new temp repo and assign it an env variable and after the test a clean up would happen. However, with Authentication we need logical isolation within a test, meaning when you spin up an orchestrator node it needs to be in one temp directory and the compute node one needs to be in other directory. The framework does not allow that. Also doing simply "export BACALHAU_DIR=$(mktemp -d)" does not work because you then also have to think of clean up. This needs more thought. Moreover with configuration pattern change now we do need to set the tokens differently for different node. Hence ideally the tests are completely wrong at the moment and the bashtub framework does not support writing test for such use case. So the task is no longer about getting it work now there is added work to support these type of tests. |
|
|
||
| var bErr bacerrors.Error | ||
| if errors.As(err, &bErr) { | ||
| // Print error message | ||
| cmd.PrintErrln(output.RedStr("Error: ") + bErr.Error()) | ||
| cmd.Println(output.RedStr("Error: ") + bErr.Error()) |
PR Details