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.

Sign up for GitHub

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

Jump to bottom

test: Add unit tests for Graph Query Builder #1729

Merged
merged 1 commit into from
Aug 17, 2021
Merged

test: Add unit tests for Graph Query Builder #1729

merged 1 commit into from
Aug 17, 2021

Conversation

mohaemen
Copy link
Contributor

Fixes #1166

@trafico-bot trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Aug 16, 2021
@CLAassistant
Copy link

CLAassistant commented Aug 16, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@mohaemen mohaemen changed the title Add unit tests for Graph Query Builder fix : Add unit tests for Graph Query Builder Aug 16, 2021
@mohaemen mohaemen marked this pull request as draft August 16, 2021 13:01
@tomkerkhove
Copy link
Owner

tomkerkhove commented Aug 16, 2021 •
edited
Loading

Thanks for the PR! You can ignore the build issues on the container image, this is something I need to fix - Sorry!

Let me know when it's ready for review.

@mohaemen
Copy link
Contributor Author

Thanks for the PR! You can ignore the build issues on the container image, this is something I need to fix - Sorry!

Let me know when it's ready for review.

Alright, no worries, I wasn't sure about them, so I moved the PR to draft.
Will move to PR again.

@mohaemen mohaemen marked this pull request as ready for review August 16, 2021 14:20
@tomkerkhove tomkerkhove changed the title fix : Add unit tests for Graph Query Builder tests : Add unit tests for Graph Query Builder Aug 16, 2021
@tomkerkhove
Copy link
Owner

No problem! The new tests are failing though. Would you mind adding an OS-agnostic way for the new lines please? Our tests run on LInux.

tomkerkhove
tomkerkhove approved these changes Aug 17, 2021
View reviewed changes
Copy link
Owner

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a ton!

@trafico-bot trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Aug 17, 2021
@tomkerkhove
Copy link
Owner

@mohaemen It looks like the commits are done with 2 users, would you mind force pushing with @mohaemen as a user please? Otherwise the CLA check won't pass.

@trafico-bot trafico-bot bot added 🔍 Ready for Review Pull Request is not reviewed yet and removed ✅ Approved Pull Request has been approved and can be merged labels Aug 17, 2021
@mohaemen
Copy link
Contributor Author

@tomkerkhove Yeah, the push was from work laptop :D
I forced pushed with my github user

@tomkerkhove tomkerkhove changed the title tests : Add unit tests for Graph Query Builder test: Add unit tests for Graph Query Builder Aug 17, 2021
@tomkerkhove tomkerkhove merged commit 6ffc6eb into tomkerkhove:master Aug 17, 2021
@trafico-bot trafico-bot bot added the ✨ Merged Pull Request has been merged successfully label Aug 17, 2021
@tomkerkhove
Copy link
Owner

Thank you!

@trafico-bot trafico-bot bot removed the 🔍 Ready for Review Pull Request is not reviewed yet label Aug 17, 2021
@mohaemen
Copy link
Contributor Author

@tomkerkhove No worries 👍

@mohaemen
Copy link
Contributor Author

@tomkerkhove one thing I noticed while working on this, the Operator.DoesNotEquals, shouldn't it be Operator.DoesNotEqual without the 's' at the end?

@tomkerkhove
Copy link
Owner

That can be an improvement, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomkerkhove tomkerkhove tomkerkhove approved these changes

Assignees
No one assigned
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide unit tests for Azure Resource Graph query builder
3 participants
@mohaemen @CLAassistant @tomkerkhove