[#115] analyze fails when nothing was successfully analyzed - #137
Conversation
The analyze command returned exit code 0 unconditionally once it ran, so a run in which every file was skipped or failed - a Player build without TypeTrees, for example - reported success and left a fully-formed but empty database behind. Both signals a caller can check said the analysis worked. Analyze now returns 1 when no file was processed successfully, explains on stderr why the run produced nothing, and discards the empty database so the output file is not mistaken for a result. Runs that processed at least one file still return 0, even when other files in the input failed. SQLiteWriter disables connection pooling so that disposing it really releases the file, and owns the discard.
| } | ||
|
|
||
| // An empty database left behind lets a caller that only checks for the output file mistake | ||
| // this run for a success. |
There was a problem hiding this comment.
the position of this comment doesn't make sense.
i don't think its a very important comment, but it should be at the call to write.Discard.
| m_ContentLayoutPath = Path.Combine(TestContext.CurrentContext.TestDirectory, | ||
| "Data", "LeadingEdgeBuilds", "BuildReport-ContentDirectory", "ContentLayout.json"); | ||
| m_AssetBundlePath = Path.Combine(TestContext.CurrentContext.TestDirectory, | ||
| "Data", "LeadingEdgeBuilds", "AssetBundles", "assetbundleroot"); |
There was a problem hiding this comment.
it is very weird to be using AssetBundles in the tests related to contentlayout. ContentLayout.json only applies to ContentDirectory builds.
Take a closer look at the reason you added these.
If we need an extra "valid" file as input then it could be a ContentDirectory from LeadingEdgeBuilds, or a BuildReport file.
E.g.
"Data\LeadingEdgeBuilds\BuildReport-ContentDirectory\f64157fb08bb9f645971d39c1203bd03.buildreport"
| var databasePath = SQLTestHelper.GetDatabasePath(m_TestOutputFolder); | ||
|
|
||
| Assert.AreEqual(0, await Program.Main(new string[] { "analyze", bundlePath, "-o", databasePath })); | ||
| Assert.AreEqual(0, await Program.Main(new string[] { "analyze", m_AssetBundlePath, "-o", databasePath })); |
There was a problem hiding this comment.
This existing test was already doing a weird mix of AssetBundles and ContetnLayout, that is establishing a bad influence for further test work in this file.
When intentionally mixing a ContentLayout.json with another file it makes more sense to import a BuildREport from a ContentDirectory build
we have one in our test data here:
TestCommon\Data\LeadingEdgeBuilds\BuildReport-ContentDirectory\f64157fb08bb9f645971d39c1203bd03.buildreport
ContentLayout.json only applies to ContentDirectory builds, so pairing it with an AssetBundle was misleading. The tests that need a second, valid input now use the build report of the same ContentDirectory build. Also moves a comment next to the call it explains.
1 participant
Summary
Fixes #115.
analyzereturned exit code 0 unconditionally once it started running, regardless of what happenedto the input files. Analyzing a Player build without TypeTrees skipped every file, wrote a
fully-formed but empty database, and exited 0 — so both signals a caller can check (the exit code,
and the existence of the
.db) reported success for a run that produced nothing. This matters mostfor scripts and AI agents driving the tool unattended.
analyzenow returns 1 when no file was processed successfully, says on stderr why the run producednothing, and discards the empty database so the output file cannot be mistaken for a result. Partial
failures are deliberately unchanged: a run that analyzed at least one file still returns 0, because
an analyze scan can legitimately pick up unexpected files (as discussed in the issue).
Changes
Analyzer
AnalyzerTool.Analyzereturns 1 whencountSuccess == 0, printsError: no files were successfully analyzed. Discarding the empty database., and adds the count offiles skipped for missing TypeTrees when that applies. This also covers an input that matched no
files at all.
SQLiteWritergainsDiscard()(close, then delete the file) and disables connection pooling, sodisposing it really releases the file. The writer already created and owned the file; deletion
belongs with it.
Tests
AnalyzeExitCodeTests: a zero-success run removes the database of a previous run, and a mixedinput (one good bundle, one without TypeTrees) still returns 0 and keeps its database.
UnityDataToolPlayerDataTestsnow expect exit 1, andAnalyze_WithPatternNoMatch_DatabaseEmptybecame
Analyze_WithPatternNoMatch_FailsWithoutDatabase.AnalyzeContentLayoutTestscases that analyze a deliberately brokenContentLayout.jsonnow also pass an AssetBundle, so the run has something to analyze and the database survives for
their schema assertions.
Testing
dotnet test— full suite green (435 + 62 + 292 passing).analyze TestCommon/Data/PlayerNoTypeTree -o out.dbexits 1 with noout.dbleft behind;analyze TestCommon/Data/AssetBundles/2019.4.0f1 -o good.dbstill exits 0 and keepsgood.db.🤖 Generated with Claude Code