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

chore: remove 3 hostname tests that are no longer relevant #229

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

sighphyre
Copy link
Member

This removes 3 of the hostname strategy tests that are unlikely to ever work correctly here and probably no longer belong. This logic is now owned by Yggdrasil and has relevant tests covering the behaviour that these 3 tests were previously testing.

Usually not a fan of removing tests to make things work but:

  1. These tests now effectively test the underlying library
  2. They require setting the hostname env variable. Doing this through Java only the affects JVM, not the Rust code
  3. At least one test remains that proves the behaviour works across the FFI boundary (gets the hostname in Java without modification, Rust gets the same hostname later)

Copy link
Member

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

Please remove the so and jar files before committing this.

Copy link
Member

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

As long as these binaries stay on branch only, I'm 👍 on removing this.

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

Makes sense as the SDK is no longer reading the hostname, but Yggdrasil is doing so, these tests at most should be inside Yggdrasil to make sure it can handle such cases (I imagine the tests are there for a reason)

@sighphyre sighphyre merged commit 08a29e5 into yggdrasil-engine-cleanup Nov 29, 2023
2 of 5 checks passed
@sighphyre sighphyre deleted the yggdrasil-hostname-fix branch November 29, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants