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

Feat: Add Unittests #54

Merged
merged 13 commits into from
Sep 9, 2024
Merged

Feat: Add Unittests #54

merged 13 commits into from
Sep 9, 2024

Conversation

zongqichen
Copy link
Contributor

No description provided.

__tests__/unittest/templates.test.js Outdated Show resolved Hide resolved
__tests__/unittest/templates.test.js Outdated Show resolved Hide resolved
Comment on lines 5 to 7
const test$schemaDefaultValue = 'https://sap.github.io/open-resource-discovery/spec-v1/interfaces/Document.schema.json';
it('should return default value', () => {
expect(defaults.$schema).toEqual(test$schemaDefaultValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's have a meeting on this how we generally want to write unit-tests.

I think a test like this is not giving us more value than a snapshot test (that we already have), but is more cumbersome to maintain. For the "isEqual" tests, I think we can just rely on the Snapshot test to ensure that we don't accidentally change something without first reviewing and accepting the change.

For the real unit-tests, I would prefer to have them test more logical things, like that there's no "undefined" string somewhere in a generated ORD document (we had such issues creep in before), or that the ORD ID of the package assignment can be resolved within the ORD document.

We could test equals assumptions where it's really critical or has special meaning, like that the default policy level of default is "sap:base:v1".

@zongqichen , @ductaily , @aramovic79

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Fannon, thank you for valuable comments. We need a meeting discussing about unittest and code style with team agreement. From my understanding and experiences, uniitest should be simple, quick, fragile, readable and understandable. If the new codes break the unittest, we have a chance to think twice, either the issue from codes or tests. This is the front safeguard for us. And maintaining unittest should be as same important as production code, we should not worry about maintaining test.

The "snapshot test" you mentioned is more like integration test for me. The purpose of integration test is to watch how it looks like in general at the end with different environment settings or complex scenarios, which unittest can not accomplish.

Comparing with snapshot test, I feel the test you mentioned has more values in different scenarios:

  • The fine-grained unittest is easier and quicker to locate the issue. And during development, we can also run the fine-grained test independently, I think it's better to debug as well.
  • Later, I want to clean and refactor the code for fixing naming issue, reducing function complexity and duplicable. The fine-grained unittest can support this process.
  • I have same feeling with you. This PR is more like uniitest proposal and skeleton. We need to improve it with more test cases and more logics. Later, I'm pretty sure, we can reuse snapshot to reduce the line of codes. Back to this test, the target function is simply enough, I assume more flexible and testcases will be enough.

btw:
I found the bug undefined during writing unittest:

// TODO: fix undefined. Root cause: get the value from ${entity['@ODM.entityName']} without setting default value
, it's missing error handling or default condition.

@zongqichen zongqichen merged commit 5b14cc7 into main Sep 9, 2024
3 checks passed
@zongqichen zongqichen deleted the add_unittest branch September 9, 2024 07:22
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