-
Notifications
You must be signed in to change notification settings - Fork 40
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
Unit test for the components #43
Conversation
Signed-off-by: Abinand P <[email protected]>
Signed-off-by: Abinand P <[email protected]>
test: fixed some test erros and added the coverage command in
Signed-off-by: Abinand P <[email protected]>
Signed-off-by: Abinand P <[email protected]>
Signed-off-by: Abinand P <[email protected]>
…, tokenType Signed-off-by: Abinand P <[email protected]>
Signed-off-by: Abinand P <[email protected]>
@quest-bot loot #12 |
Quest PR submitted!@Abiji-2020, you are attempting to solve the issue and loot this Quest. Will you be successful? Questions? Check out the docs. |
@Abiji-2020, yes, I would wait for #37 and then merge from main to ensure we have 90% coverage. |
oh okayy, |
No, we still waiting for Jay to submit the CR changes |
@gemanor do we need tests for the hooks?? |
Signed-off-by: Abinand P <[email protected]>
Signed-off-by: Abinand P <[email protected]>
All the components have their individual unit tests, and also the complete flow test by combining it, the major reduce in coverage is due to hooks and
even though they are commented they are included in the coverage |
You can remove these files @Abiji-2020 |
And to your question, yes, hooks has critical logic in it. All the hooks that being used, should have tests |
coverage increased to 90% |
@gemanor could you take a look at this? |
Hey @Abiji-2020, the tests have not been passed on my local machine. Can you check that? This is the result:
|
I guess you have not stored any key in the keychain so it is missing let me check and update that |
@Abiji-2020, I extend the delay and now it passed locally, but not in the CI |
Can we mock it instead? Having CI private versions could lead to security vulnerabilities and it's anti-pattern |
I have gone through it but it is a problem with the package |
Signed-off-by: Abinand P <[email protected]>
It seems like it worked, but we still have problem with the keytar itself. Any idea? |
we can install the libsecret-1.so.0 package in the ci? or else we need to mock keytar in all of them |
What could be the implication of installing it in the CI? |
Signed-off-by: Abinand P <[email protected]>
I added it on main, so you can revert the ci changes you made.. |
Signed-off-by: Abinand P <[email protected]>
All of them are working on the windows machine so let me check them and update them if any errors. |
Okay, now it all failed. I think it is better to mock the keytar functions too.. |
I reverted the CI files, I think installing the lib-secret will just create more troubles. It's okay to mock the keytar IMO |
@Abiji-2020 please let me know when you have a version with keytar functions mocked, and I'll re-run the CI tests |
ok @gemanor will update |
Signed-off-by: Abinand P <[email protected]>
@Abiji-2020, this is a good progress, seems like we are mostly good with it |
yes 3 more test files should be modified . |
Signed-off-by: Abinand P <[email protected]>
@gemanor updated and fixed CI |
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.
LGTM
🧚 @Abiji-2020 congratulations for completing Quest #12 💰 A reward of $250 has been credited to you. To claim your $250 reward follow the instructions here. Questions? Check out the docs. |
Unit test for the components
Currently Pending components
Coverage report
/claim #12
closes #12