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

Unbreak test suites with multiple files loading common constants #904

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

debarshiray
Copy link
Contributor

Test suites with a helper file with common constants that is loaded by
multiple files stopped working with:

  $ bats /some/path
  /some/path/test_helper.bash: line 1: A_CONSTANT: readonly variable
  Error while sourcing library loader at '/some/path/test_helper.bash'

If the helper file had anything else other than the common constants,
then the tests would run, but the spew about the constants would still
remain:

  $ bats --tap /some/path
  /some/path/test_helper.bash: line 1: A_CONSTANT: readonly variable
  1..2
  ok 1 first
  ok 2 second

Fallout from 9d5ecdb

@debarshiray debarshiray requested a review from a team as a code owner April 19, 2024 18:51
Some editors like GNU Emacs insist on adding the newline at end of file,
if it's missing, which leads to noise when looking at the actual code
changes.

bats-core#904
debarshiray added a commit to debarshiray/bats-core that referenced this pull request Apr 19, 2024
Test suites with a helper file with common constants that is loaded by
multiple files stopped working with:
  $ bats /some/path
  /some/path/test_helper.bash: line 1: A_CONSTANT: readonly variable
  Error while sourcing library loader at '/some/path/test_helper.bash'

If the helper file had anything else other than the common constants,
then the tests would run, but the spew about the constants would still
remain:
  $ bats --tap /some/path
  /some/path/test_helper.bash: line 1: A_CONSTANT: readonly variable
  1..2
  ok 1 first
  ok 2 second

Fallout from 9d5ecdb

bats-core#904
debarshiray added a commit to debarshiray/bats-core that referenced this pull request Apr 19, 2024
@debarshiray debarshiray force-pushed the wip/rishi/unbreak-suite-load-readonly branch from b94660b to 745b619 Compare April 19, 2024 18:52
@debarshiray
Copy link
Contributor Author

This goes on top of #902

@debarshiray debarshiray mentioned this pull request Apr 19, 2024
2 tasks
Copy link
Member

@martin-schulze-vireso martin-schulze-vireso left a comment

Choose a reason for hiding this comment

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

Thanks for your report and suggestion. I believe that we should not ignore load errors during the test registration, that will only lead to hard to diagnose problems.

IMHO, the correct way forward should be to separate the evaluationen of individual test files during test registration in the same way as is done for test execution.

@debarshiray
Copy link
Contributor Author

Thanks for your report and suggestion. I believe that we should not ignore load errors during the test registration, that will only lead to hard to diagnose problems.

Yeah, I can imagine.

IMHO, the correct way forward should be to separate the evaluationen of individual test files during test registration in the same way as is done for test execution.

I don't understand the Bats code well enough to know how exactly that should be done. I can try it, if you give me some pointers, or I don't mind if you do it yourself and I hope that the tests would still be useful in that case. :)

@debarshiray
Copy link
Contributor Author

Thanks for the review!

Test suites with a helper file with common constants that is loaded by
multiple files stopped working with:
  $ bats /some/path
  /some/path/test_helper.bash: line 1: A_CONSTANT: readonly variable
  Error while sourcing library loader at '/some/path/test_helper.bash'

If the helper file had anything else other than the common constants,
then the tests would run, but the spew about the constants would still
remain:
  $ bats --tap /some/path
  /some/path/test_helper.bash: line 1: A_CONSTANT: readonly variable
  1..2
  ok 1 first
  ok 2 second

Fallout from 9d5ecdb

bats-core#904
@debarshiray debarshiray force-pushed the wip/rishi/unbreak-suite-load-readonly branch from 745b619 to f94e695 Compare April 22, 2024 16:10
@debarshiray
Copy link
Contributor Author

Tried to fix the changelog and shellcheck tests.

debarshiray added a commit to debarshiray/toolbox that referenced this pull request Apr 30, 2024
Ansible's built-in 'package' module doesn't show any details when
installing the RPMs.  All that can be seen is:
  TASK [Install RPM packages]
  fedora-rawhide | changed

Therefore, there's no way to know what version of the packages got
installed.

In this case, not knowing the Bats version being used by the CI makes it
difficult to know why the tests are generating this spew on Fedora
Rawhide [1]:
  TASK [Run system tests]
  test/system/libs/helpers.bash: line 7: TEMP_BASE_DIR: readonly variable
  test/system/libs/helpers.bash: line 8: TEMP_STORAGE_DIR: readonly variable
  test/system/libs/helpers.bash: line 10: IMAGE_CACHE_DIR: readonly variable
  test/system/libs/helpers.bash: line 11: ROOTLESS_PODMAN_STORE_DIR: readonly variable
  test/system/libs/helpers.bash: line 12: ROOTLESS_PODMAN_RUNROOT_DIR: readonly variable
  test/system/libs/helpers.bash: line 13: PODMAN_STORE_CONFIG_FILE: readonly variable
  test/system/libs/helpers.bash: line 14: DOCKER_REG_ROOT: readonly variable
  test/system/libs/helpers.bash: line 15: DOCKER_REG_CERTS_DIR: readonly variable
  test/system/libs/helpers.bash: line 16: DOCKER_REG_AUTH_DIR: readonly variable
  test/system/libs/helpers.bash: line 17: DOCKER_REG_URI: readonly variable
  test/system/libs/helpers.bash: line 18: DOCKER_REG_NAME: readonly variable
  test/system/libs/helpers.bash: line 21: PODMAN: readonly variable
  test/system/libs/helpers.bash: line 22: TOOLBX: readonly variable
  test/system/libs/helpers.bash: line 23: SKOPEO: readonly variable
  ...
  fedora-rawhide | 1..340

[1] bats-core/bats-core#904
debarshiray added a commit to debarshiray/toolbox that referenced this pull request Apr 30, 2024
Ansible's built-in 'package' module doesn't show any details when
installing the RPMs.  All that can be seen is:
  TASK [Install RPM packages]
  fedora-rawhide | changed

Therefore, there's no way to know what version of the packages got
installed.

In this case, not knowing the Bats version being used by the CI makes it
difficult to know why the tests are generating this spew on Fedora
Rawhide [1]:
  TASK [Run system tests]
  test/system/libs/helpers.bash: line 7: TEMP_BASE_DIR: readonly variable
  test/system/libs/helpers.bash: line 8: TEMP_STORAGE_DIR: readonly variable
  test/system/libs/helpers.bash: line 10: IMAGE_CACHE_DIR: readonly variable
  test/system/libs/helpers.bash: line 11: ROOTLESS_PODMAN_STORE_DIR: readonly variable
  test/system/libs/helpers.bash: line 12: ROOTLESS_PODMAN_RUNROOT_DIR: readonly variable
  test/system/libs/helpers.bash: line 13: PODMAN_STORE_CONFIG_FILE: readonly variable
  test/system/libs/helpers.bash: line 14: DOCKER_REG_ROOT: readonly variable
  test/system/libs/helpers.bash: line 15: DOCKER_REG_CERTS_DIR: readonly variable
  test/system/libs/helpers.bash: line 16: DOCKER_REG_AUTH_DIR: readonly variable
  test/system/libs/helpers.bash: line 17: DOCKER_REG_URI: readonly variable
  test/system/libs/helpers.bash: line 18: DOCKER_REG_NAME: readonly variable
  test/system/libs/helpers.bash: line 21: PODMAN: readonly variable
  test/system/libs/helpers.bash: line 22: TOOLBX: readonly variable
  test/system/libs/helpers.bash: line 23: SKOPEO: readonly variable
  ...
  fedora-rawhide | 1..340

[1] bats-core/bats-core#904

containers#1482
@martin-schulze-vireso
Copy link
Member

I underestimated the complexity. The code for counting tests and duplicates relied on having everything evaluated in the same shell. This will need a rework but I am out of time for today.

@debarshiray
Copy link
Contributor Author

I underestimated the complexity. The code for counting tests and duplicates relied on having everything evaluated in the same shell. This will need a rework but I am out of time for today.

Thanks for working on this, @martin-schulze-vireso !

@martin-schulze-vireso martin-schulze-vireso force-pushed the wip/rishi/unbreak-suite-load-readonly branch from 0775227 to 23f55be Compare May 6, 2024 22:24
@martin-schulze-vireso martin-schulze-vireso force-pushed the wip/rishi/unbreak-suite-load-readonly branch from 23f55be to 85f73c2 Compare May 6, 2024 22:50
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.

None yet

2 participants