test: add missing unit tests for project utils path conversion - #457
test: add missing unit tests for project utils path conversion#457Ayush4958 wants to merge 1 commit into
Conversation
Adds unit test coverage for absoluteToRelativePathForAssetGroups and relativeToAbsolutePathForAssetGroups in ProjectUtil, resolving existing TODO comments.
lrasmus
left a comment
lrasmus
left a comment
There was a problem hiding this comment.
The tests make sense conceptually but I think they are going to give the wrong idea about expected behavior. Please see more detailed comment on request for change. Thanks!
| }); | ||
|
|
||
| it('should call AssetUtil.absoluteToRelativePathForArray for each asset group', () => { | ||
| const spy = jest.spyOn(AssetUtil, 'absoluteToRelativePathForArray').mockImplementation((p, a) => a); |
There was a problem hiding this comment.
I realize the intent of this test isn't to actually verify the functionality of absoluteToRelativePathForArray (we have other tests for that), however my concern is that by mocking the function so it changes how it behaves, this test (if someone were to read it as documentation) would get the wrong idea of what this function does.
I think we need the original absoluteToRelativePathForArray functionality to be applied. The result of calling the function under test should never be the same as the input, and this is what the test says it expects.
Same for the relative -> absolute tests too.
| ProjectUtil.removeExternalAsset(project, asset); | ||
| expect(project.externalAssets.children.length).toEqual(1); | ||
| }); | ||
| describe('absoluteToRelativePathForAssetGroups', () => { |
There was a problem hiding this comment.
These test blocks are embedded within describe('removeExternalAsset', () => { - could we please move them out of that block?
2 participants
Description
I had resolves two
// TODO - unit tests!!comments inapp/utils/project.jsby adding test coverage for the following methods:ProjectUtil.absoluteToRelativePathForAssetGroupsProjectUtil.relativeToAbsolutePathForAssetGroupsChanges Made
describeblock for both methods intest/utils/project.spec.jsensuring proper null/undefined handling and verifying delegation toAssetUtil.AssetUtilinto the test suite.TODOcomments fromapp/utils/project.js.