Skip to content

Conversation

@DariaBod
Copy link
Contributor

@DariaBod DariaBod commented Feb 5, 2026

Rationale

Some changes to support multi choice

Related Pull Requests

Changes

  • Added setAllowMultipleSelections method to check “Allow multiple selections” checkbox

  • Changed method initFilterColumn for work with filters for List values

  • Added method testMultiChoiceValues() to ListTest (scope: create, edit)

  • Created new shuffleSelect() for random amount of random values from list

  • Changed method selectOptionByText() for multi choice values. If we need value to be selected we first check if it’s selected and then click

@DariaBod DariaBod changed the title Fb mvtc test Tests for multi choice value Feb 5, 2026
@LabKey LabKey deleted a comment from github-actions bot Feb 5, 2026
-add new shuffleSelect
-replace my method with shuffleSelect
@DariaBod DariaBod requested a review from labkey-tchad February 5, 2026 23:18
Copy link
Contributor

@labkey-danield labkey-danield left a comment

Choose a reason for hiding this comment

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

Added a few comments.

Comment on lines +239 to +242
if(textChoiceValidator.getMultipleSelections())
{
fieldRow.setAllowMultipleSelections(textChoiceValidator.getMultipleSelections());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will only set it if textChoiceValidator.getMultipleSelections is true, correct? This is ok if you are creating a field, but wouldn't work as expected if the test is editing an existing field and wants to change to field from a multi-choice to a single choice.
Perhaps a better check would be:
if(null != textChoiceValidator.getMultipleSelections())

Comment on lines +516 to +517
lookupSelect.clearSelection();

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work if a test is trying to add to an existing selections?

Comment on lines +239 to +242
if(textChoiceValidator.getMultipleSelections())
{
fieldRow.setAllowMultipleSelections(textChoiceValidator.getMultipleSelections());
}
Copy link
Member

Choose a reason for hiding this comment

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

This would make it impossible to disable multi-select through this method.
This should either call setAllowMultipleSelections unconditionally or the condition should check whether textChoiceValidator.getMultipleSelections() != null.

Comment on lines 52 to 59
/**
* Select a single facet value by clicking its label. Should replace all existing selections.
* @param value desired value
*/
public void selectFilter(String value)
{
elementCache().filterTypeSelects.select(value);
}
Copy link
Member

Choose a reason for hiding this comment

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

Update this comment to describe this method (looks leftover from the selectValue). The comment should also mention that this is only relevant to multi-value text choice fields.

This could also take a Filter.Operator instead of a String for some extra type-safety.

List<String> values = (List<String>) value;
filterModal.selectFacetTab().selectValue(values.get(0));
filterModal.selectFacetTab().checkValues(values.toArray(String[]::new));
filterModal.selectFacetTab().selectFilter(operator.getDisplayValue());
Copy link
Member

Choose a reason for hiding this comment

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

I think this will (or could) break functionality for other column types; the facet panel usually doesn't have a filter type.
Also, this wouldn't work for "Is Empty" or "Is Not Empty".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m planning to add support for 'Is Empty' and 'Is Not Empty' as well.

Regarding the functionality for other types: I’ve already found one test that was failing, so I added a check to the initFilterColumn method. It now checks if the filter type selector is present on the page before trying to interact with it. After this change, the test is passing.

Does this approach sound okay to you?

for (Map.Entry<String, String> entry : values.entrySet())
{
WebElement fieldInput = Locator.name(EscapeUtil.getFormFieldName(entry.getKey())).findElement(getDriver());
WebElement fieldInput = Locator.nameContaining(EscapeUtil.getFormFieldName(entry.getKey())).findElement(getDriver());
Copy link
Member

Choose a reason for hiding this comment

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

Use a more precise locator and add a comment explaining why this can't match the name exactly. (See UpdateQueryRowPage)

Comment on lines +1705 to +1710
table.clickInsertNewRow();
String valuesToChoose = tcValues.subList(1, 3).stream()
.sorted()
.collect(Collectors.joining(" "));
Locator loc = Locator.nameContaining(EscapeUtil.getFormFieldName(columnName));
selectOptionByText(loc, valuesToChoose);
Copy link
Member

Choose a reason for hiding this comment

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

clickInsertNewRow returns a page object that should have methods for setting fields.

Comment on lines +1696 to +1698
String encodedListName = "multiChoiceList";
String keyName = "'><script>alert(\":(\")</script>'";
String columnName = "MultiChoiceField";
Copy link
Member

Choose a reason for hiding this comment

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

Consider using randomly generated field and list names. (I'm not sure whether ListHelper can handle the tricky characters but it's worth a try)

Suggested change
String encodedListName = "multiChoiceList";
String keyName = "'><script>alert(\":(\")</script>'";
String columnName = "MultiChoiceField";
String encodedListName = TestDataGenerator.randomDomainName("multiChoiceList", DomainUtils.DomainKind.IntList);
String keyName = TestDataGenerator.randomFieldName("'><script>alert(\":(\")</script>'");
String columnName = TestDataGenerator.randomFieldName("MultiChoiceField");

selectOptionByText(loc, valuesToChoose);

clickButton("Submit");
checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName));
Copy link
Member

Choose a reason for hiding this comment

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

checker() doesn't take screenshots automatically. If you're doing a single check, checker().withScreenshot().verifyX will trigger a failure screenshot. If you're doing several checks, you should call checker().screenShotIfNewError(); after the checks.

Suggested change
checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName));
checker().withScreenshot().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName));

clickButton("Submit");

// verify the multi choice value is persisted
checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName));
Copy link
Member

Choose a reason for hiding this comment

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

Take a screenshot.

Suggested change
checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName));
checker().withScreenshot().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName));

Comment on lines +4062 to +4068
if(Boolean.parseBoolean(selectElement.getAttribute("multiple"))) {
List<WebElement> elems = selectElement.findElements(Locator.tag("option"));
elems.forEach(element->{
if(value.contains(element.getAttribute("value")) ^ element.isSelected()) element.click();
});
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is less precise than we like for broadly available helpers; it would get confused by selection options that have overlapping text. I would suggest having a separate method for multi-select (selectOptionsByText(WebElement selectElement, List<String> values).

Comment on lines +1693 to +1694
public void testMultiChoiceValues()
{
Copy link
Member

Choose a reason for hiding this comment

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

We should only run the test on Postgres.

Suggested change
public void testMultiChoiceValues()
{
public void testMultiChoiceValues()
{
Assume.assumeTrue("Multi-choice text fields are only supported on PostgreSQL", WebTestHelper.getDatabaseType() == WebTestHelper.DatabaseType.PostgreSQL);

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.

5 participants