Page MenuHome

Add unit tests for asset library suitable root path function
Needs RevisionPublic

Authored by Julian Eisel (Severin) on Sep 29 2021, 12:43 PM.

Details

Summary

Adds unit tests for functionality added in D12647: Assets: Expose option to reuse data-block data when appending.

For now only testing BKE_asset_library_find_suitable_root_path_from_filepath(). Not sure how to test BKE_asset_library_find_suitable_root_path_from_main() best. It's just a wrapper around the former but takes Main *. I think we'd have to duplicate the test logic, which is not great.

Diff Detail

Repository
rB Blender
Branch
asset_library_test_find_suitable_root_path_from_filepath (branched from master)
Build Status
Buildable 17440
Build 17440: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) requested review of this revision.Sep 29 2021, 12:43 PM
Julian Eisel (Severin) created this revision.
  • Update to changes in master
  • Fix memory leak

Remove unrelated changes (hopefully...)

Julian Eisel (Severin) updated this revision to Diff 42666.EditedSep 29 2021, 3:17 PM

And another attempt... there we go.

Sybren A. Stüvel (sybren) requested changes to this revision.Oct 1 2021, 10:54 AM

I've triggered a test build of this patch at https://builder.blender.org/admin/#/builders/18/builds/127
It should be checked on Windows before committing.

source/blender/blenkernel/intern/asset_library_test.cc
94

This should be documented a bit; a constructor manipulating a global object is not something that's trivial.

118–119

Below you use SEP, so also change "/" to use SEP instead.

121–122

Don't call things retval. It's a horrible name in itself, and here it doesn't even follow the vague-and-should-be-shot-down convention of "the value returned by this function".

This revision now requires changes to proceed.Oct 1 2021, 10:54 AM
Julian Eisel (Severin) marked 2 inline comments as done.
  • Improve function comment, test Windows and ".." paths

The tests are rather extensive now, but it should cover plenty of cases now, also with Windows paths.

source/blender/blenkernel/BKE_asset_library.h
35–71

Can commit this separately. Nice thing is that the tests helped me making the API specification more specific :)

Sybren A. Stüvel (sybren) requested changes to this revision.Oct 11 2021, 6:15 PM

Just a few small notes.

source/blender/blenkernel/intern/asset_library_test.cc
103–105

Why does this remove all asset libraries? Why not just the ones that were added with the function below? If it doesn't matter, why even back up U?

124

ALTSEP is / on Windows, so basically this is sort-of-but-not-quite BLI_path_slash_native(). Is there a reason to not use that function, but to do this in this way instead? And why is the name library_path_win when on Windows it will be more like a POSIX path?

141

Use EXPECT_TRUE(was_successful) instead.

145–148

Are two identically-named asset libraries supported? Wouldn't that break the asset library reference system?

167

Add a comment that explains that the resulting path is still inside the asset library, as this isn't immediately obvious without scrolling up & counting path components.

This revision now requires changes to proceed.Oct 11 2021, 6:15 PM