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

SALTO-6995: Add element-test-utils package #6866

Merged

Conversation

ori-moisis
Copy link
Contributor

The first thing this package implements is an override for jest expect that compares element IDs using a custom matcher


Additional context for reviewer
Required for #6862


Release Notes:
None


User Notifications:
None

@coveralls
Copy link

coveralls commented Nov 28, 2024

Coverage Status

coverage: 93.813%. remained the same
when pulling 1fe4aaf on ori-moisis:feature/add_element_test_utils
into 68fe957 on salto-io:main.

Copy link
Contributor

@tamtamirr tamtamirr left a comment

Choose a reason for hiding this comment

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

Nice 👑 Added some comments

Comment on lines 12 to 19
// We cannot import this from @salto-io/element-test-utils because it would cause a cyclic dependency
// so we have to define this here separately
export const isEqualElemID: Tester = (a, b) => {
if (a instanceof ElemID && b instanceof ElemID) {
return a.isEqual(b)
}
return undefined
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Do we a have future ticket to separate the types from adapter-api to another package in order to break this cycle?
  • Is there any way to enforce having all the matchers from elements-test-utils here?

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 can't think of any way that would allow this given that this code requires the class (for instanceof, for calling isEqual, etc...).
AFAICT there is no way to break the cycle, because the whole point of this new package is that it can use things from adapter-api (unlike the general test-utils).


---

This package contains a few useful shortcuts and utilities for the [jest framework](https://jestjs.io/)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should write a simple conventions guide about how to add matchers to this package.

@ori-moisis ori-moisis force-pushed the feature/add_element_test_utils branch from 71a9160 to 25d9f5f Compare December 1, 2024 16:44
The first thing this package implements is an override for jest expect that compares element IDs using a custom matcher
@ori-moisis ori-moisis merged commit 3aa9092 into salto-io:main Dec 3, 2024
61 checks passed
@ori-moisis ori-moisis deleted the feature/add_element_test_utils branch December 3, 2024 08:46
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.

3 participants