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

FEDX-126 FED-1719 Null Safety Migration #63

Merged
merged 33 commits into from
Nov 28, 2023
Merged

FEDX-126 FED-1719 Null Safety Migration #63

merged 33 commits into from
Nov 28, 2023

Conversation

sydneyjodon-wk
Copy link
Contributor

@sydneyjodon-wk sydneyjodon-wk commented Jun 16, 2023

Note: Merging is dependent on react-dart null safety migration completion. ✅

This null safety migration is almost complete. With the following remaining todos:

  • Get CI passing
    • Remaining test failures are due to an attribute issue (see comment below)
  • Do some consumer testing

QA Instructions

@aviary2-wf
Copy link

aviary2-wf commented Jun 16, 2023

Security Insights

The items listed below may not capture all security relevant changes. Before providing a security review, be sure to review the entire PR for security impact.

(1) Security relevant changes were detected
  • Watched keyword innerHtml in lib/src/matchers/jest_dom/is_empty_dom_element.dart line(s) ['70'] added
  • Action Items

    • Obtain a security review; reviewer should pay special attention to insights listed above
    • Verify aviary.yaml coverage of security relevant code

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

    @sydneyjodon-wk sydneyjodon-wk changed the title Null Safe Migration Null Safety Migration Jun 16, 2023
    # Conflicts:
    #	.github/workflows/dart_ci.yml
    @greglittlefield-wf greglittlefield-wf changed the title Null Safety Migration FEDX-126 Null Safety Migration Jun 23, 2023
    @sydneyjodon-wk
    Copy link
    Contributor Author

    @aaronlademann-wf I think I addressed all your comments - I went back through and checked a lot of typing with the JS typing

    @aaronlademann-wf
    Copy link
    Contributor

    @sydneyjodon-wk looks like the highcharts consumer PR has a merge conflict, and the w_filing PR has failing tests.

    All the others are passing, so I "checked them off" in your description.

    So close!

    @aaronlademann-wf
    Copy link
    Contributor

    QA +1

    • CI is passing
    • All consumer tests listed in the description are passing

    @aaronlademann-wf
    Copy link
    Contributor

    Semver +1

    • This is expected to be a major release
    • Rosie's version after merge is correct as 3.0.0

    Copy link
    Contributor

    @greglittlefield-wf greglittlefield-wf left a comment

    Choose a reason for hiding this comment

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

    Just found a few things, with a good chunk of them just being test cleanliness.

    lib/src/matchers/jest_dom/has_form_values.dart Outdated Show resolved Hide resolved
    lib/src/user_event/user_event.dart Outdated Show resolved Hide resolved
    lib/src/user_event/user_event.dart Outdated Show resolved Hide resolved
    lib/src/util/console_log_formatter.dart Outdated Show resolved Hide resolved
    lib/src/util/error_message_utils.dart Outdated Show resolved Hide resolved
    test/unit/dom/queries/by_testid_test.dart Outdated Show resolved Hide resolved
    test/unit/dom/queries/by_text_test.dart Outdated Show resolved Hide resolved
    test/unit/dom/top_level_queries_test.dart Show resolved Hide resolved
    test/unit/dom/wait_for_test.dart Outdated Show resolved Hide resolved
    test/unit/util/matchers.dart Outdated Show resolved Hide resolved
    @greglittlefield-wf
    Copy link
    Contributor

    semver +1

    @kealjones-wk
    Copy link
    Contributor

    kealjones-wk commented Nov 28, 2023

    Security +1

    • Watched keyword innerHtml in lib/src/matchers/jest_dom/is_empty_dom_element.dart line(s) ['70'] added

      • This is safe usage as it is not setting the innerHtml and it is also only doing boolean check to see if that text is empty and doing nothing else with it.

      For future reference you could instead use .textContent and it will return the same content without tripping up aviary.

    @sydneyjodon-wk
    Copy link
    Contributor Author

    @Workiva/release-management-p

    @aaronlademann-wf
    Copy link
    Contributor

    QA +1 refresh

    • CI Passing
    • Relevant Consumer Tests Passing

    Copy link

    @rmconsole-wf rmconsole-wf left a comment

    Choose a reason for hiding this comment

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

    +1 from RM

    @rmconsole7-wk rmconsole7-wk merged commit 7a74c64 into master Nov 28, 2023
    5 checks passed
    @rmconsole7-wk rmconsole7-wk deleted the null-safety branch November 28, 2023 22:59
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    8 participants