-
Notifications
You must be signed in to change notification settings - Fork 85
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
Added Test for Reload #265
Added Test for Reload #265
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are just a few minor comments to be addressed in the test, but overall it looks good.
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
Thanks done @marcin-krystianc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sammychinedu2ky,
one more thing. Since we rely on the CONSUL_AGENT_CONFIG_PATH
to be set, we need to set it in the CI workflow so the test is actually run 😄
Alright 👌 |
.github/workflows/ci.yml
Outdated
@@ -126,6 +126,7 @@ jobs: | |||
- name: Run tests | |||
shell: bash | |||
run: | | |||
export CONSUL_AGENT_CONFIG_PATH=$GITHUB_WORKSPACE/Consul.Test/test_config.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using export, you can use env
context in line 133
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched it now 😄
Hi @marcin-krystianc , some test cases passed and some didn't
and in some instances it said
|
Hi @marcin-krystianc please do you know the reason behind that |
@marcin-krystianc . I think I would use the skippable feature here |
Makes sense. |
done @marcin-krystianc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
Fixes #264
@marcin-krystianc @naskio