Skip to content

Restore custom suppliers in TestDataGenerator#1976

Merged
labkey-chrisj merged 3 commits intorelease24.7-SNAPSHOTfrom
24.7_fb_customSuppliers
Jul 11, 2024
Merged

Restore custom suppliers in TestDataGenerator#1976
labkey-chrisj merged 3 commits intorelease24.7-SNAPSHOTfrom
24.7_fb_customSuppliers

Conversation

@labkey-chrisj
Copy link
Copy Markdown
Contributor

Rationale

Recently on 24.7, BiologicsReportTest.testBarChartWithAggregateMethod began failing because the chart-builder expects there to be values in the field it is using for its y-axis column. The reason that column was empty, it happens, is that this domain was being generated using a custom dataSupplier (its job was to put the value '1' in generated rows.)

My recent changes to TestDataGenerator seem to have inadvertently regressed its ability to use custom dataSuppliers when generating row data. This change initializes the default field data-generating suppliers to the list of suppliers if no supplier is in the list, and uses suppliers from the list (vs. calling getDefaultDataSupplier every row to get a supplier) to generate data.

Related Pull Requests

#1971

Changes

  • Initialize suppliers in the suppliers list for fields to receive generated data, and use suppliers from the list instead of getting default suppliers only

@github-actions

This comment was marked as outdated.

@labkey-chrisj labkey-chrisj changed the base branch from develop to release24.7-SNAPSHOT July 10, 2024 20:15
Comment on lines +260 to +263
if (!_dataSuppliers.containsKey(columnName))
{
_dataSuppliers.put(columnName, getDefaultDataSupplier(_columns.get(columnName).getRangeURI()));
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sick Map trick 🛹

Suggested change
if (!_dataSuppliers.containsKey(columnName))
{
_dataSuppliers.put(columnName, getDefaultDataSupplier(_columns.get(columnName).getRangeURI()));
}
_dataSuppliers.computeIfAbsent(columnName, k -> getDefaultDataSupplier(_columns.get(columnName).getRangeURI()));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was excited to try this sick map trick, but unfortunately it didn't update the suppliers map; this meant that below the call to

_dataSuppliers.get(columnName).get();

...would fail because _dataSuppliers.get(columnName) would be null ☹️

Comment on lines +269 to +270
// Generate values
Object columnValue = _dataSuppliers.get(columnName).apply(_rows);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making _dataSuppliers actually be Suppliers. We don't actually use the row collection in any existing data suppliers and I don't know that it adds much utility to provide it.

Map<String, Supplier<Object>> _dataSuppliers = new CaseInsensitiveHashMap<>()

Then this simplifies to:

Suggested change
// Generate values
Object columnValue = _dataSuppliers.get(columnName).apply(_rows);
// Generate values
Object columnValue = _dataSuppliers.get(columnName).get();

And we could remove the unused method addDataSupplier(String columnName, Function<List<Map<String, Object>>, Object> supplier)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I like this suggestion a lot. (applying it to _rows without really understanding why has bothered me, this gets me out of that)
Also, this makes everything else that interacts with _dataSuppliers a lot more readable

Co-authored-by: Trey Chadick <tchad@labkey.com>
@labkey-chrisj labkey-chrisj merged commit 0455e61 into release24.7-SNAPSHOT Jul 11, 2024
@labkey-chrisj labkey-chrisj deleted the 24.7_fb_customSuppliers branch July 11, 2024 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants