Skip to content

Test fixes#27412

Open
rrjjvv wants to merge 3 commits intohashicorp:mainfrom
rrjjvv:test-fixes
Open

Test fixes#27412
rrjjvv wants to merge 3 commits intohashicorp:mainfrom
rrjjvv:test-fixes

Conversation

@rrjjvv
Copy link
Copy Markdown

@rrjjvv rrjjvv commented Jun 8, 2024

I recently opened #27404 and had a possible fix, but make test had failures. These changes are not at all related to each other, but they seemed trivial enough to combine and to skip issue creation. I'd be happy to split this apart and/or create a dedicated issue if I'm wrong.

For the AWS change, my first instinct was to make those changes "higher up" (e.g. in the Makefile or in the build scripts), but I chose to be conservative. I'm a long-time user of Vault, but I'm new to the build/test process.

For the consul test, the bug report that led to this test (#23013) was more likely to occur with Enterprise, which is likely why that image was used. I do know the test passes using the OSS image, but I don't know if that reduces the usefulness of the test. I can get a license, but I saw (unofficial) comments in places that suggested that "regular devs" aren't expected to have a license when running the tests locally. I could not find any obvious mechanism for ignoring enterprise-specific tests, so, for this change, I chose to assume:

  1. tests should not require a license to run
  2. there is still value in running the test using OSS, even if it doesn't fully reflect the original environment (vs. skipping the test when a license is not present)
  3. those points outweigh the risk of "works on my machine" (where someone gets different results based on presence/absence of a license)

rrjjvv added 3 commits June 7, 2024 21:37
The test reads these if they exist.  They likely don't exist in CI, but
my files were read and changed the test's behavior by not returning the
expected error response.
@rrjjvv rrjjvv requested a review from a team as a code owner June 8, 2024 05:00
@hashicorp-cla-app
Copy link
Copy Markdown

hashicorp-cla-app bot commented Jun 8, 2024

CLA assistant check
All committers have signed the CLA.

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.

1 participant