Page MenuHomePhabricator

Allow unit tests to have arbitrarily long names (>255 characters)
ClosedPublic

Authored by epriestley on Feb 15 2019, 3:19 PM.
Tags
None
Referenced Files
F14116049: D20180.diff
Thu, Nov 28, 1:35 PM
Unknown Object (File)
Mon, Nov 25, 2:18 PM
Unknown Object (File)
Sun, Nov 24, 2:35 AM
Unknown Object (File)
Fri, Nov 22, 9:30 AM
Unknown Object (File)
Fri, Nov 22, 3:41 AM
Unknown Object (File)
Oct 26 2024, 4:52 PM
Unknown Object (File)
Oct 25 2024, 1:12 AM
Unknown Object (File)
Oct 17 2024, 6:18 PM
Subscribers
Restricted Owners Package

Details

Summary

Depends on D20179. Ref T13088. See PHI351. See PHI1018. In various cases, unit tests names are 19 paths mashed together.

This is probably not an ideal name, and the test harness should probably pick a better name, but if users are fine with it and don't want to do the work to summarize on their own, accept them. We may summarize with "..." in some cases depending on how this fares in the UI.

The actual implementation is a separate "strings" table which is just <hash-of-string, full-string>. The unit message table can end up being mostly strings, so this should reduce storage requirements a bit.

For now, I'm not forcing a migration: new writes use the new table, existing rows retain the data. I plan to provide a migration tool, recommend migration, then force migration eventually.

Prior to that, I'm likely to move at least some other columns to use this table (e.g., lint names), since we have a lot of similar data (arbitrarily long user string constants that we are unlikely to need to search or filter).

Test Plan

Screen Shot 2019-02-15 at 7.14.22 AM.png (1×2 px, 348 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a subscriber: Restricted Owners Package.Feb 15 2019, 3:19 PM

Can't wait for unit tests like

Screen Shot 2019-02-15 at 1.01.54 PM.png (578×1 px, 137 KB)

amckinley added inline comments.
src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php
279–280

Should this catch block be scoped more tightly than just Exception? I'm worried about what else we might end up eating.

This revision is now accepted and ready to land.Feb 19 2019, 7:13 PM
src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php
285–287

We throw anything we catch down here anyway, this is just because I want to finally { $this->name = $old_name; } but we don't have finally until PHP 5.5.

If save() fails for whatever reason, I just want to avoid removing the object's name as a surprising side effect.

This revision was automatically updated to reflect the committed changes.