Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test Automation for Date-only and Time-only Fields #1836

Merged
merged 40 commits into from
Mar 18, 2024

Conversation

labkey-danield
Copy link
Contributor

@labkey-danield labkey-danield commented Feb 23, 2024

Rationale

Test automation for the date-only and time-only fields.

I honestly tried to put the new date-only and time-only tests into ListTest. However the ListTest is an older tests and several "one-off" checks have been added to it that made it difficult to insert new tests. Several of the older tests use a globally defined list that made it very difficult to add new columns to without causing other tests to fail. Several tests used column positions as part of validation which also made adding new fields difficult. I cleaned up the List test a bit but it was still easier to add the date and time only tests in a separate file.

This change also include a consolidation of the site "look and feel" page with the folder "configuration" page. Some of the tests for this story needed to validate the reconciliation of formate setting in site vs. folder. Consolidating the two pages made the tests cleaner.

@XingY The new test cases for date & time only fields are:

There are also several data files that have been updated I'll leave up to you if want to review.

The ListTest was also change but was primarily for clean up, using new helpers, etc... There are no specific date and time only tests in this file.

I have TeamCity run that exercises all of theses tests.

@labkey-tchad I believe you have already reviewed the setting page changes. I haven't changed them since the initial draft review. For completeness the changes mostly focused on the form change are:

DiscussionLinkTest.java was updated to use the new settings page.

A new method, setTypeWithDialog, was added to DomainFieldRow.java. This was added to accommodate the test that validate changing a dateTime field to a date or time only field.

Related Pull Requests

Changes

  • Added DomainFieldRow.setTypeWithDialog. This returns the dialog so the warning message when converting a field can be validated.
  • Consolidated the settings page for Site and Folder.
    • Created BaseSettingsPage which has the shared functionality between the site and folder settings page.
    • Changed LookAndFeelSettingsPage to derive from BaseSettingsPage
    • Changed ProjectSettingsPage to derive from BaseSettingsPage
    • Had to update a few tests because of this change.
  • Added tests to ProjectSettingsTest to validate site and folder settings are scoped correctly.
  • Changed VarListDefinition.addField to validate that the assumed key field is of a valid key type.
  • Added date-only and time-only tests to ParsingPatternForDateTest
  • Added ListDateAndTimeTest.
  • "Cleaned up" ListTest
    • This test needs to be refactored.
  • Added AssayTest
    • Validate date and time only fields in results and run fields.
  • Added DataClasses
    • Possibly redundant with the list tests and may not add much value.

Add a date and time fields to ParsingPatternForDateTest.java
Update ProjectSettingsTest to actually check the menu.
Add time and date format fields to setting page.
Updated test to use consolidated page.
Clean up test that uses additional pattern parsing.
Replaced a DateTime field with a TextChoice field.
Moved checks for DateTime field into separate tests.
Added tests for import and sorting date-only and time-only fields.
Added a check to VarListDefinition to validate that the assumed field to be the key is of the correct type.
… It was making the test too large and unmanageable.

Added tests for formatting and bad values validation.
…to validate warning message.

Add test that converts a DateTime field to a date-only field and a time-only field.
Copy link
Contributor

@XingY XingY left a comment

Choose a reason for hiding this comment

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

Will there be a separate story for checking date/time values in Assay Run domains? Samples/dataclass can be checked additionally in apps, dataset can probably be skipped. But it would be good to have some test coverage for Assay Run fields since its backend implementation is different

}

private void setUpDataParsing(String pattern)
private void setAdditionalParsingPatterns(String dateTimePattern, String datePattern, String timePattern)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any coverage for site/project level setting for those fields?

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 have added tests that validate parsing mode set at site and project levels.

String pattern = "ddMMMyyyy:HH:mm:ss";
setUpDataParsing(pattern);
log("Setting the additional parsing patterns.");
String dateTimePattern = "ddMMMyyyy:HH:mm:ss";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have test coverage for US vs non-US parsing mode?

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've added a test for non-US parsing.


List<Date> dates = new ArrayList<>();

dates.add(new Calendar.Builder()
Copy link
Contributor

Choose a reason for hiding this comment

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

For regression testing for issue 47341, it would be useful to have a datetime value with actual time portion that's before 1970. The test can test 2 things: before converting to time field, export will export datetime as expected. After converting to time, the column in UI and exported are both as expected. This can be a separate test case.

Copy link
Contributor Author

@labkey-danield labkey-danield Mar 11, 2024

Choose a reason for hiding this comment

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

There are test dates that happen before 1970 (Date-only, Time-only and DateTime values). I think the test date is 1952, or sometime in the 50's, so we should be good there.

I've added test that validate the round trip import / export issue with excel called out in issue 47341. The roundtrip does include dates before 1970.

click(Locator.tagWithText("h3", "Upload file (.xlsx, .xls, .csv, .txt)"));
click(Locator.tagContainingText("h3", "Upload file"));

// Note: the date and time columns in this xlsx are formatted as date and time types. Excel does not have a date-time format, that is a string.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. Date/Time/Datetime in excel are all "Numbers", with different format. The field should be a "Number" type, not a string type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though it would be good to have separate coverage that test import from excel with "String" type date/time/datetime too.

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 have added a xlsx file for import that has Date, Time and DateTime fields in text format.

@labkey-danield labkey-danield marked this pull request as ready for review March 11, 2024 22:37
setAssayResultsProperties(assayDesignerPage, 11);

DomainFormPanel domainFormPanel = assayDesignerPage.expandFieldsPanel("Run Fields");
domainFormPanel.addField(new FieldDefinition("Date", FieldDefinition.ColumnType.Date));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there coverage for converting assay run fields from datetime -> date/time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is now.

Copy link
Member

@labkey-tchad labkey-tchad left a comment

Choose a reason for hiding this comment

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

There are a lot of places that could be sped up by using APIs

Comment on lines 116 to 132
private void prepForDateAndTimeOnlyTest(String listName) throws IOException, CommandException
{
log("Reset site setting to have default formats for date and time values.");
LookAndFeelSettingsPage settingsPage = LookAndFeelSettingsPage.beginAt(this);
settingsPage.reset();

// Reset doesn't wait for page (page may or may not refresh).
sleep(500);

// Use java's Date object and SimpleDateFormatter (not strings) to enter the data.
_defaultDateFormat = new SimpleDateFormat(settingsPage.getDefaultDateDisplay());
_defaultTimeFormat = new SimpleDateFormat(settingsPage.getDefaultTimeDisplay());
_defaultDateTimeFormat = new SimpleDateFormat(settingsPage.getDefaultDateTimeDisplay());

deleteListIfPresent(getProjectName(), listName);

}
Copy link
Member

Choose a reason for hiding this comment

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

This does a lot of unnecessary UI groveling. Only getting the default formats requires going through the UI and that only needs to happen once (during doSetup would be good). The rest of this (resetting the look and feel and deleting the list) can be done via API.

Copy link
Contributor Author

@labkey-danield labkey-danield Mar 14, 2024

Choose a reason for hiding this comment

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

Changed.

…n assay run.

Fixed some typos.
Removing use of AfterClass method.
Adding a site look and feel reset to TestScrubber.
Copy link
Member

@labkey-tchad labkey-tchad left a comment

Choose a reason for hiding this comment

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

Still a lot of places that could be sped up by using APIs. ListDateAndTimeTest creates a lot of lists via UI.

Comment on lines 145 to 146
new SimplePostCommand("admin", "resetProperties")
.execute(createDefaultConnection(), "/");
Copy link
Member

Choose a reason for hiding this comment

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

Move this to a static method in BaseSettingsPage to be shared with test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 91 to 94
catch (IOException | CommandException e)
{
TestLogger.error("Failed to reset site look and settings.", e);
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this error should be ignored. Swallowing the exception could lead to hard-to-diagnose failures.

Copy link
Contributor Author

@labkey-danield labkey-danield Mar 14, 2024

Choose a reason for hiding this comment

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

Changed to throw a new runtime exception.

Comment on lines 104 to 107
catch (IOException | CommandException e)
{
TestLogger.error("Failed to reset project settings.", e);
}
Copy link
Member

Choose a reason for hiding this comment

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

Rethrow this instead of ignoring it.

Suggested change
catch (IOException | CommandException e)
{
TestLogger.error("Failed to reset project settings.", e);
}
catch (IOException | CommandException e)
{
throw new RuntimeException("Failed to reset project settings.", e);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

Comment on lines 245 to 250
goToProjectHome();
if(goToManageLists().getGrid().getListNames().contains(listName))
{
_listHelper.goToList(listName);
_listHelper.deleteList();
}
Copy link
Member

Choose a reason for hiding this comment

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

Did DomainUtils.ensureDeleted(getProjectName(), "lists", listName); not work for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did, but I must have missed this reference or undid the change as I was doing other updates.
Added it back.

Comment on lines 178 to 201
private void createDateAndTimeList(String project, List<Map<String, String>> listData)
{
_listHelper.createList(project, DT_LIST_NAME, ListHelper.ListColumnType.AutoInteger, DT_LIST_ID_COL,
new FieldDefinition(DT_LIST_DATE_COL, FieldDefinition.ColumnType.Date),
new FieldDefinition(DT_LIST_TIME_COL, FieldDefinition.ColumnType.Time),
new FieldDefinition(DT_LIST_DATETIME_COL, FieldDefinition.ColumnType.DateAndTime));


// Use the default format to initially populate the lists.
StringBuilder bulkImportData = new StringBuilder();
bulkImportData.append(String.format("%s\t%s\t%s\n", DT_LIST_DATE_COL, DT_LIST_TIME_COL, DT_LIST_DATETIME_COL));

for(Map<String, String> listRow : listData)
{
bulkImportData.append(String.format("%s\t%s\t%s\n",
listRow.get(DT_LIST_DATE_COL),
listRow.get(DT_LIST_TIME_COL),
listRow.get(DT_LIST_DATETIME_COL))
);
}

_listHelper.bulkImportData(bulkImportData.toString());

}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a list or domain designer focused test, it shouldn't spend a bunch of time in UI that is tested elsewhere. Use API helpers for list creation (IntListDefinition) and data import (TestDataGenerator).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

Comment on lines 142 to 152
private void resetSiteSettings()
{
LookAndFeelSettingsPage settingsPage = LookAndFeelSettingsPage.beginAt(this);
settingsPage.reset();
}

private void resetProjectSettings()
{
ProjectSettingsPage settingsPage = ProjectSettingsPage.beginAt(this, PROJ_CHANGE);
settingsPage.reset();
}
Copy link
Member

Choose a reason for hiding this comment

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

Use API to reset site/project settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

Comment on lines 46 to 48
private SimpleDateFormat _defaultDateFormat = new SimpleDateFormat("yyyy-MM-dd");
private SimpleDateFormat _defaultTimeFormat = new SimpleDateFormat("HH:mm:ss");
private SimpleDateFormat _defaultDateTimeFormat = new SimpleDateFormat("yyyy-MM-dd HH:mm");
Copy link
Member

Choose a reason for hiding this comment

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

It is misleading to give these initial values since they get defined in doSetup. They also need to be static for tests to see the value set in doSetup (each @Test is run with a different instance of the test class).

Suggested change
private SimpleDateFormat _defaultDateFormat = new SimpleDateFormat("yyyy-MM-dd");
private SimpleDateFormat _defaultTimeFormat = new SimpleDateFormat("HH:mm:ss");
private SimpleDateFormat _defaultDateTimeFormat = new SimpleDateFormat("yyyy-MM-dd HH:mm");
// Fetched from look and feel page in 'doSetup'
private static SimpleDateFormat _defaultDateFormat = null;
private static SimpleDateFormat _defaultTimeFormat = null;
private static SimpleDateFormat _defaultDateTimeFormat = null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

int rowIndex = 0;
for(Map<String, String> expectedRowMap : expectedData)
{
// Protect against expecting more rows than are present in the grid.
Copy link
Member

Choose a reason for hiding this comment

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

Why protect against this? You've already verified that the table has the correct number of rows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it does not get an index out of bounds error and stops the entire tests.

import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebElement;

public class BaseSettingsPage extends LabKeyPage<LabKeyPage<?>.ElementCache>
Copy link
Member

Choose a reason for hiding this comment

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

If you change how this is declared, you won't have to override elementCache below.

Suggested change
public class BaseSettingsPage extends LabKeyPage<LabKeyPage<?>.ElementCache>
public class BaseSettingsPage extends LabKeyPage<BaseSettingsPage.ElementCache>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@labkey-danield labkey-danield merged commit bb131fe into develop Mar 18, 2024
2 checks passed
@labkey-danield labkey-danield deleted the fb_dateAndTimeAutomation branch March 18, 2024 20:12
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