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

Add support for picking RD_LOCATION for BATS #4573

Merged
merged 1 commit into from
May 2, 2023

Conversation

jandubois
Copy link
Member

@jandubois jandubois commented Apr 28, 2023

Choose from system/user/dist/npm or pick first available. dist and npm only work when running from inside a git clone. npm is not yet supported on Windows.

Also move "load ../helpers/load" from setup() to top level, so we can abort on fatal errors.

Predefine teardown_file() with rdctl shutdown so it doesn't have to be repeated in every test file.

Partially implements #4542 but more work is needed.

Additional comments

./bats-core/bin/bats tests/* passed successfully on macOS Intel for both "moby" and "containerd": 158 tests, 0 failures, 5 skipped.

You can tell BATS to use the package in the dist directory even when RD is also installed globally, or per user:

RD_LOCATION=dist ./bats-core/bin/bats tests/*

I'm not really happy with the RD_LOCATION name, but couldn't think of anything better. I briefly considered RD_INSTALL, but that seems even worse.

Using RD_LOCATION=npm has been tested with the registry/creds.bats test, which does run a second factory-reset in the middle:

$ RD_LOCATION=npm ./bats-core/bin/bats tests/registry/creds.bats
creds.bats
[...]

19 tests, 0 failures

The factory reset logic should be made more robust (e.g. I've seen it once that limactl/qemu were not killed), and needs to be implemented for Windows. This should happen in a separate PR.

The apify_arg() function should be auto-generated from the schema in command-api.yaml. This too should happen in a separate PR.

bats/tests/helpers/defaults.bash Show resolved Hide resolved
bats/tests/helpers/load.bash Show resolved Hide resolved
bats/tests/helpers/paths.bash Show resolved Hide resolved
bats/tests/helpers/paths.bash Show resolved Hide resolved
bats/tests/helpers/paths.bash Outdated Show resolved Hide resolved
bats/tests/helpers/paths.bash Outdated Show resolved Hide resolved
bats/tests/helpers/paths.bash Outdated Show resolved Hide resolved
@@ -21,11 +32,32 @@ factory_reset() {
fi
}

# Turn `rdctl start` arguments into `npm run dev` arguments
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 consider making npm run dev take the same arguments as rdctl start instead. That's better for our sanity. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

That works for me too, but should be a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, definitely not as part of this PR.

@jandubois jandubois force-pushed the rd-location branch 2 times, most recently from 70d2241 to 629f3ed Compare May 1, 2023 18:25
@jandubois jandubois requested a review from mook-as May 1, 2023 19:53
@ericpromislow
Copy link
Contributor

ericpromislow commented May 1, 2023

I see the same breakage on the main branch in macOS, so something else is going on outside this PR.

I get this breakage on macOS after running `bats bats/tests/k8s/wordpress.bats bats/tests/registry/creds.bats` in both `main` and this branch with `~/.docker/config.json` set to ``` { "credsStore": "none" } ```

Breakage:

/Users/ericp/workspace/rancher/desktop/bats/tests/k8s/wordpress.bats
 ✓ factory reset
 ✓ add helm repo
 ✓ start rancher desktop
 ✓ deploy wordpress
 ✓ verify wordpress was deployed
/Users/ericp/workspace/rancher/desktop/bats/tests/registry/creds.bats
 ✗ factory reset
   (from function `factory_reset' in file bats/tests/helpers/vm.bash, line 17,
    in test file bats/tests/registry/creds.bats, line 82)
     `factory_reset' failed
   kill -9 90658
   kill -9 90668
   kill -9 90672
   kill -9 90853
   91287
   91293
   91303
   91304
   91307
   91311
   91315
   92382
 ✓ start container engine
 ✓ wait for container engine
 ✗ verify credential is set correctly
   (from function `assert_output' in file bats/bats-assert/src/assert_output.bash, line 194,
    from function `verify_default_credStore' in file bats/tests/registry/creds.bats, line 107,
    in test file bats/tests/registry/creds.bats, line 100)
     `verify_default_credStore' failed

   -- output differs --
   expected : osxkeychain
   actual   : none
   --

If I run bats bats/tests/registry/creds.bats in isolation I don't get the error -- for some reason the credsStore field gets set to osxkeychain at the start of the run

I don't see this breakage when running in main.

I've run both sequences three times in both branches and this happens consistently.

Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

See note on breakage in #4573 (comment)

@jandubois
Copy link
Member Author

See note on breakage in #4573 (comment)

Please provide details on how to reproduce breakage; it works for me.

Also, if this is using RD_LOCATION=npm, then it should be fixed in a follow-up PR; this PR is about the structural changes needed so you can specify RD_LOCATION.

bats/tests/k8s/wordpress.bats Outdated Show resolved Hide resolved
ericpromislow
ericpromislow previously approved these changes May 2, 2023
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Because the breakage I saw on this PR is also visible in main, there's no reason to hold this PR back.

mook-as
mook-as previously approved these changes May 2, 2023
bats/tests/helpers/vm.bash Outdated Show resolved Hide resolved
Choose from system/user/dist/npm or pick first available.
dist and npm only work when running from inside a git clone.
npm is not yet supported on Windows.

Also move "load ../helpers/load" from setup() to top level,
so we can abort on fatal errors.

Predefine teardown_file() with `rdctl shutdown` so it doesn't
have to be repeated in every test file.

Signed-off-by: Jan Dubois <jan.dubois@suse.com>
@jandubois jandubois merged commit ed9230f into rancher-sandbox:main May 2, 2023
@jandubois jandubois deleted the rd-location branch May 2, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants