CPLAT-14309 - adding support for multiple testIds per element #11

brianphillips-wk · 2021-05-24T19:58:28Z

Thread:
https://workiva.slack.com/archives/GFELW7RRB/p1621465412019400

Ticket:
https://jira.atl.workiva.net/browse/CPLAT-14309

aviary2-wf · 2021-05-24T20:00:04Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

rmconsole-wf · 2021-05-24T20:00:27Z

Merge Requirements Met ✅

Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment.

General Information

Ticket(s):

CPLAT-14309 | remove

Code Review(s): #11
Release Image Tags:

Reviewers: aaronlademann-wf, sydneyjodon-wk

Additional Information

Watchlist Notifications: None

	When this pull is merged I will add it to the following release:
	Current version: react_testing_library 0.3.3
	Version after merge: react_testing_library 0.3.3
	Release Ticket(s): RM-104146

	This pull is considered a release pull
	The options defined for this repo will be carried out

Note: This is a shortened report. Click here to view Rosie's full evaluation.
Last updated on Friday, May 28 03:08 PM CST

semveraudit-wf · 2021-05-24T20:01:29Z

Public API Changes

No changes to the public API found for commit f8cfe96

Showing results for f8cfe96

Powered by semver-audit-service. Please report any problems by filing an issue.
Reported by the dart semver audit client 2.2.1
Browse public API.

Last edited UTC May 27 at 13:56:51

aaronlademann-wf

Looks like current tests will need to be updated to expect the "regex" flavor of failure message instead of a string one. Those tests are nested pretty deep in a bunch of shared test logic though, so if you want me to pitch in there or pair up with you - lets do it.

Also - you need to format changes before you commit each time. You can either run pub run dart_dev format in your terminal, or I can show you how to set that up to just happen on file save in your IDE.

aaronlademann-wf · 2021-05-24T20:00:41Z

test/unit/dom/queries/by_testid_test.dart

+              'data-test-id': 'testId1 testId2',
+            }, 'Testing multiple'),
+            react.span({
+              'data-test-id': 'single',
+            }, 'Testing single')) as ReactElement);
+      });
+
+      test('[match]', () {
+        expect(renderResult.getByTestId('testId1'), isA<SpanElement>());
+        expect(renderResult.getByTestId('testId2'), isA<SpanElement>());
+        expect(renderResult.getByTestId('single'), isA<SpanElement>());
+        expect(
+            renderResult.getByTestId(RegExp('single')), isA<SpanElement>());
+      });


I think it would be a good idea to have a third test id in there as well to assert that the match doesn't have to be at the beginning or end of the attribute value.

I'd also add a test that involves test id's that have characters like - / _, etc.

+ So that the user doesn’t see an error for a RegExp value they didn’t provide, and so we don’t have to rewrite a ton of test expectations.

sydneyjodon-wk

Looks great! Just had a couple comments on tests.

test/unit/dom/queries/by_testid_test.dart

sydneyjodon-wk · 2021-05-25T18:16:13Z

lib/src/dom/queries/by_testid.dart

+    try {
+      return jsQuery(TextMatch.toJs(testId));
+    } catch (e) {
+      errorCaughtUsingStringTestIdValue = e;
+
+      try {
+        // Try using a regex to do a word match within the attribute value in case the element has multiple test ids.
+        return jsQuery(_convertTestIdStringToRegExp(testId, exact: exact));
+      } catch (_) {
+        // Even the string converted to regex didn't match, which means the string passed was not found at all.
+        // Throw the original error that was thrown as a result of the string provided since that is how the
+        // user authored it so that the failure message displayed to the user doesn't contain a strange regex
+        // that they didn't write.
+        throw TestingLibraryElementError.fromJs(errorCaughtUsingStringTestIdValue);
+      }
+    }
+  }


What if there we have something like this where one element has just one test id and the other has multiple:

react.span({ 'data-test-id': 'testId3', }, 'Testing single'), react.div({ 'data-test-id': 'testId3 testId4', }, 'Testing allBy')

won't this just return the span because an error wouldn't have been thrown so it never gets the chance to add the regex and get the div as well?

Can there be a test case for something like that?

Great catch, you're right—this doesn't account for those test cases. This also affects error throws if ...ByTestId detects multiple. Looking into this. Thanks Sydney

sydneyjodon-wk · 2021-05-25T18:18:14Z

lib/src/dom/queries/by_testid.dart

+    if (testId is String && jsQueryResult.isEmpty) {
+      return jsQuery(_convertTestIdStringToRegExp(testId, exact: exact));
+    }


Same question as above for this logic.

lib/src/dom/queries/by_testid.dart

…o CPLAT-14309-getTestId-multiple-ids

…LAT-14309-getTestId-multiple-ids

brianphillips-wk · 2021-05-26T20:05:57Z

I added (and continuing to add) the full range of tests for test-id queries so ensure we're not losing base functionality as we add on this multi-ID check.

Still a few failing tests (outside of by_testid-test.dart), so investigating that. We tried adding a try...catch to account for test result mismatches on some older tests, but that caused some troubles on the core logic of this fix. Werkin on it!

This reverts commit 8fb4c75. # Conflicts: # lib/src/dom/queries/by_testid.dart

…s_adl Provide error messages with string test ids as authored to consumers

sydneyjodon-wk

+10

CI passes
Good test coverage

test/unit/dom/configure_test.dart

aaronlademann-wf

+1

@Workiva/release-management-pp

rmconsole-wf · 2021-05-28T20:05:35Z

@aaronlademann-wf I will not merge this because:

'All commits reviewed' is False
Last commit does not have code review

sydneyjodon-wk

@Workiva/release-management-p

rmconsole-wf

+1 from RM

added regex check for testId strings

7bec44d

brianphillips-wk requested review from greglittlefield-wf, aaronlademann-wf and sydneyjodon-wk May 24, 2021 19:58

brianphillips-wk self-assigned this May 24, 2021

rmconsole-wf added the Merge Requirements Unmet label May 24, 2021

brianphillips-wk removed their assignment May 24, 2021

semveraudit-wf added the semver: Patch label May 24, 2021

aaronlademann-wf requested changes May 24, 2021

View reviewed changes

aaronlademann-wf added 3 commits May 24, 2021 13:54

Format

c4bcf64

Add clarifying comment

3aa71fd

Rethrow the first error

9733e50

+ So that the user doesn’t see an error for a RegExp value they didn’t provide, and so we don’t have to rewrite a ton of test expectations.

sydneyjodon-wk reviewed May 24, 2021

View reviewed changes

test/unit/dom/queries/by_testid_test.dart Outdated Show resolved Hide resolved

test/unit/dom/queries/by_testid_test.dart Outdated Show resolved Hide resolved

test/unit/dom/queries/by_testid_test.dart Outdated Show resolved Hide resolved

brianphillips-wk added 4 commits May 25, 2021 12:29

refactoring and added tests for testId queries

06dacf2

added testTextMatchTypes reference comment

6d1cd6c

added tests with '-' characters

1325e10

added additional special character to test

517838b

brianphillips-wk removed the request for review from greglittlefield-wf May 25, 2021 17:07

formatted files

743f4a9

sydneyjodon-wk requested changes May 25, 2021

View reviewed changes

brianphillips-wk and others added 7 commits May 25, 2021 14:27

removed errant comment

9e2cf0c

Merge branch 'master' of github.com:Workiva/react_testing_library int…

95ff457

…o CPLAT-14309-getTestId-multiple-ids

Increase test timeout

7bbdfaa

added all tests for basic testId functionality

d270636

added all test cases for new and old functionality

870cb06

rolled back changes for catching errors

8fb4c75

Merge branch 'CPLAT-14309-getTestId-multiple-ids--full-tests' into CP…

d643da0

…LAT-14309-getTestId-multiple-ids

formatting

5fed11e

aaronlademann-wf and others added 3 commits May 26, 2021 15:33

Revert "rolled back changes for catching errors"

9f89174

This reverts commit 8fb4c75. # Conflicts: # lib/src/dom/queries/by_testid.dart

Tweak error catching

e5c79e6

Merge pull request #12 from Workiva/CPLAT-14309-getTestId-multiple-id…

f8cfe96

…s_adl Provide error messages with string test ids as authored to consumers

sydneyjodon-wk approved these changes May 27, 2021

View reviewed changes

test/unit/dom/configure_test.dart Show resolved Hide resolved

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels May 27, 2021

aaronlademann-wf approved these changes May 28, 2021

View reviewed changes

rmconsole-wf added RM Ready Merge Requirements Unmet and removed Merge Requirements Met labels May 28, 2021

sydneyjodon-wk approved these changes May 28, 2021

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels May 28, 2021

rmconsole2-wf merged commit dbfc9bf into master May 28, 2021

rmconsole-wf approved these changes May 28, 2021

View reviewed changes

rmconsole2-wf deleted the CPLAT-14309-getTestId-multiple-ids branch May 28, 2021 20:08

CPLAT-14309 - adding support for multiple testIds per element #11

CPLAT-14309 - adding support for multiple testIds per element #11

Uh oh!

Conversation

brianphillips-wk commented May 24, 2021

Uh oh!

aviary2-wf commented May 24, 2021

Security Insights

Action Items

Uh oh!

rmconsole-wf commented May 24, 2021 • edited by rmconsole2-wf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Requirements Met ✅

General Information

Additional Information

Uh oh!

semveraudit-wf commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Public API Changes

Uh oh!

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Uh oh!

aaronlademann-wf May 24, 2021

Choose a reason for hiding this comment

Uh oh!

sydneyjodon-wk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sydneyjodon-wk May 25, 2021

Choose a reason for hiding this comment

Uh oh!

sydneyjodon-wk May 25, 2021

Choose a reason for hiding this comment

Uh oh!

brianphillips-wk May 25, 2021

Choose a reason for hiding this comment

Uh oh!

sydneyjodon-wk May 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brianphillips-wk commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sydneyjodon-wk left a comment • edited by brianphillips-wk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Uh oh!

rmconsole-wf commented May 28, 2021

Uh oh!

sydneyjodon-wk left a comment

Choose a reason for hiding this comment

Uh oh!

rmconsole-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rmconsole-wf commented May 24, 2021 •

edited by rmconsole2-wf

Loading

semveraudit-wf commented May 24, 2021 •

edited

Loading

brianphillips-wk commented May 26, 2021 •

edited

Loading

sydneyjodon-wk left a comment •

edited by brianphillips-wk

Loading