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

Added Test for Reload #265

Merged
merged 29 commits into from
Nov 29, 2023
Merged

Conversation

sammychinedu2ky
Copy link
Contributor

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a 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.

Consul/Interfaces/IAgentEndpoint.cs Show resolved Hide resolved
Consul.Test/AgentTest.cs Outdated Show resolved Hide resolved
Consul.Test/AgentTest.cs Outdated Show resolved Hide resolved
Consul.Test/AgentTest.cs Outdated Show resolved Hide resolved
Consul.Test/AgentTest.cs Outdated Show resolved Hide resolved
sammychinedu2ky and others added 4 commits November 3, 2023 14:29
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
@sammychinedu2ky
Copy link
Contributor Author

Thanks done @marcin-krystianc

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a 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 😄

@sammychinedu2ky
Copy link
Contributor Author

Alright 👌

@@ -126,6 +126,7 @@ jobs:
- name: Run tests
shell: bash
run: |
export CONSUL_AGENT_CONFIG_PATH=$GITHUB_WORKSPACE/Consul.Test/test_config.json
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched it now 😄

@sammychinedu2ky
Copy link
Contributor Author

sammychinedu2ky commented Nov 6, 2023

Hi @marcin-krystianc , some test cases passed and some didn't
In some instances it said

 Expected: DEBUG
Actual:   TRACE

and in some instances it said

The given key was not present in the dictionary.

@sammychinedu2ky
Copy link
Contributor Author

Hi @marcin-krystianc please do you know the reason behind that

@sammychinedu2ky
Copy link
Contributor Author

@marcin-krystianc . I think I would use the skippable feature here

@marcin-krystianc
Copy link
Contributor

@marcin-krystianc . I think I would use the skippable feature here

Makes sense.

@sammychinedu2ky
Copy link
Contributor Author

done @marcin-krystianc

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@marcin-krystianc marcin-krystianc merged commit ffcd671 into G-Research:master Nov 29, 2023
160 checks passed
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.

Added Test for Agent Reload
3 participants