Page MenuHome
Differential D12689

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

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

Details

Reviewers
Sybren A. Stüvel (sybren)
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 17441
Build 17441: 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.
Harbormaster completed remote builds in B17422: Diff 42638.Sep 29 2021, 12:43 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Sep 29 2021, 12:45 PM
Julian Eisel (Severin) added a reviewer: Sybren A. Stüvel (sybren).
Julian Eisel (Severin) added a project: Asset Browser.
Julian Eisel (Severin) edited the summary of this revision. (Show Details)
Julian Eisel (Severin) mentioned this in D12675: Support displaying catalogs in the Current File asset library.Sep 29 2021, 12:54 PM
Julian Eisel (Severin) mentioned this in rBadaf4f56e1ed: Support loading catalogs in the Current File asset library.Sep 29 2021, 1:20 PM
Julian Eisel (Severin) updated this revision to Diff 42663.Sep 29 2021, 3:13 PM
  • Update to changes in master
  • Fix memory leak
Harbormaster completed remote builds in B17438: Diff 42663.Sep 29 2021, 3:13 PM
Julian Eisel (Severin) updated this revision to Diff 42665.Sep 29 2021, 3:16 PM

Remove unrelated changes (hopefully...)

Harbormaster completed remote builds in B17440: Diff 42665.Sep 29 2021, 3:16 PM
Julian Eisel (Severin) updated this revision to Diff 42666.EditedSep 29 2021, 3:17 PM

And another attempt... there we go.

Harbormaster completed remote builds in B17441: Diff 42666.Sep 29 2021, 3:17 PM
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) updated this revision to Diff 42787.Oct 1 2021, 4:04 PM
Julian Eisel (Severin) marked 2 inline comments as done.
  • Improve function comment, test Windows and ".." paths
Harbormaster completed remote builds in B17498: Diff 42787.Oct 1 2021, 4:04 PM
Julian Eisel (Severin) added a comment.Oct 1 2021, 4:07 PM

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
43–53 ↗(On Diff #42787)

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

Julian Eisel (Severin) added a comment.Oct 4 2021, 1:59 PM

The tests succeed by the way: https://builder.blender.org/admin/#/builders/18/builds/129

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

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
intern/
85 lines

Diff 42666

View Options

source/blender/blenkernel/intern/asset_library_test.cc

Loading...