Skip to content

Improve CI workflows#4016

Merged
emlynmac merged 11 commits intomainfrom
emlyn/ci-updates
Jan 16, 2024
Merged

Improve CI workflows#4016
emlynmac merged 11 commits intomainfrom
emlyn/ci-updates

Conversation

@emlynmac
Copy link
Copy Markdown
Member

@emlynmac emlynmac commented Jan 15, 2024

What

Break out jest coverage to a separate parallel job.
Remove hard coding of node version and global rush version

Why

Improve maintainability

How Tested

CI

Process & policy checklist

  • I have updated the project documentation to reflect my changes if necessary.
  • I have read the CONTRIBUTING documentation.

Is this a breaking change?

  • This change causes current functionality to break.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 15, 2024

CallWithChat bundle size is not changed.

  • Current size: 6449387
  • Base size: 6449387
  • Diff size: 0

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 15, 2024

Chat bundle size is not changed.

  • Current size: 1438444
  • Base size: 1438444
  • Diff size: 0

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 15, 2024

Calling bundle size is not changed.

  • Current size: 5727758
  • Base size: 5727758
  • Diff size: 0

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 15, 2024

@azure/communication-react jest test coverage for beta.

Lines Statements Functions Branches
Base 44648 / 71951
62.05%
44648 / 71951
62.05%
913 / 2019
45.22%
2631 / 4244
61.99%
Current 44646 / 71951
62.05%
44646 / 71951
62.05%
913 / 2019
45.22%
2636 / 4248
62.05%
Diff -2 / 0
0%
-2 / 0
0%
0 / 0
0%
5 / 4
0.06%

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 15, 2024

@azure/communication-react jest test coverage for stable.

Lines Statements Functions Branches
Base 21414 / 34133
62.73%
21414 / 34133
62.73%
580 / 1026
56.53%
1694 / 2698
62.78%
Current 44646 / 71951
62.05%
44646 / 71951
62.05%
913 / 2019
45.22%
2636 / 4248
62.05%
Diff 23232 / 37818
-0.68%
23232 / 37818
-0.68%
333 / 993
-11.31%
942 / 1550
-0.73%

Comment thread .github/workflows/ci.yml
packages/communication-react/temp/communication-react.api.json

jest-test-coverage:
needs: [get_matrix, build_packages]
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.

As this job has its own build step, we don't need to wait for build_packages job to be completed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't need it, this is true. But there's no point running it unless the build step completes successfully

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.

Build job also includes linting and treeshaking (~2 minutes) that can be skipped for tests. Tests will finish or almost will finish their run during that time.
Or are we going to move those too out of the build job?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These aren't the critical path, so it doesn't really matter how long these tests take to run.
Yes, building also does linting too.
I tried moving that out to a pre-build job, but it doesn't make the process quicker.

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
uses: actions/setup-node@v4
with:
node-version: '20.x'
node-version: 'lts/*'
Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside Jan 15, 2024

Choose a reason for hiding this comment

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

I'm hesitant about using LTS on anything CI where we should be as stable as possible and try pinning dependencies that may shift under us. We will hit those issues where suddenly half the PRs start failing or snapshots are partially updating if the linux version bump has some big changes.

Is the purpose to improve performance (prevent the linux machines from having to take time installing the right version)? (If so, how much does it improve by? i.e. to evaluate if its worth it)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I understand we want to control the deps as much as possible.
For the case of CI though, we should be using LTS node regardless. This change keeps it in sync.

The purpose for this change is so we don't need as much maintenance time. This will always use LTS.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As an aside, we could also make this a matrix and run the whole thing against multiple node versions...

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.

For the case of CI though, we should be using LTS node regardless

Why is this? I feel we shouldn't be using LTS for CI. At all times we should have a stable development platform that devs know will work and can contribute changes that remains unblocked. Separate from that, we then upgrade our development contract when new versions of node are LTS only after we've verified new versions work.

I would definitely be in favor of action though that is kicked off nightly/automatically when we new versions of node are released and verifies CI works.

Copy link
Copy Markdown
Member Author

@emlynmac emlynmac Jan 16, 2024

Choose a reason for hiding this comment

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

The crux here is that LTS will migrate the node version as LTS roles automatically, which means there's no way it will get forgotten. It means one less thing to update.
If we're not running CI against the LTS version of Node, what do you feel we should be using and why?

If we're keeping on top of dependencies internally, then there will never be an issue. If, however, there does happen to be a break when a new version of node comes out and we have not been ready for it, we have a means to ensure it is caught.

Comment thread .github/workflows/ci.yml
- name: Type check test folder
if: ${{ matrix.flavor == 'beta' }}
run: cd packages/react-composites && rushx tsc:e2e
# # Type check test folder
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This can probably be deleted.
The job is not run within a matrix context, so the test here is always false and does not get executed. Need to confirm this was the intention

@github-actions
Copy link
Copy Markdown
Contributor

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot.

@emlynmac emlynmac merged commit cefe2d7 into main Jan 16, 2024
@emlynmac emlynmac deleted the emlyn/ci-updates branch January 16, 2024 22:43
palatter added a commit that referenced this pull request Jan 18, 2024
* [Calling sounds] Stabilize Calling sounds feature (#3969)

* stabilize sounds

* Change files

* Duplicate change files for beta release

* build beta API

* fix breaking change

* build API files

* update stable API

---------

Signed-off-by: Donald McEachern <94866715+dmceachernmsft@users.noreply.github.com>

* Fix `noInvalidAny` chat composites (#3987)

* [DTMF Dialler] Enable DTMF mode and dialing out mode in dialpad (#3970)

* remove code forcing end of number to be edit with key

* Change files

* Duplicate change files for beta release

* add editing mode for dialing and dtmf default

* disable input on click if there is no editing

* remove ref

* build API

* fix test for new API

* [InlineImage][Test] Resolve resource not found error for inline image placeholder UITest  (#3979)

* Resolve resource not found error for inline image placeholder UITest

* Update UI Testing doc

Signed-off-by: Patrick Latter <73612854+palatter@users.noreply.github.com>

* Resolve issue with inline image placeholder test

* Change files

* Duplicate change files for beta release

---------

Signed-off-by: Patrick Latter <73612854+palatter@users.noreply.github.com>

* Fix up typing on chat components (#3988)

* Replace html-to-react with html-react-parser (#3995)

* Enable noImplicitAny in chat-stateful-client (#3998)

* Enable noImplicitAny in the chat sample (#4000)

* Update component examples for noImplicitAny (#4001)

* Make Calling Sample textfield validate the Teams user id format (#3967)

* Prevent dialpad unecessarily triggering onChange event (#3963)

* [DTMF Dialer] Update Dialpad styles per designs (#3999)

* Update Dialpad styles per designs

* Change files

* Duplicate change files for beta release

* fix alignment

* Update packages/react-composites CallWithChatComposite browser test snapshots

* Update packages/react-composites CallComposite browser test snapshots

* sizing

* Update packages/react-composites CallWithChatComposite browser test snapshots

* Update packages/react-composites CallComposite browser test snapshots

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* [Calling] Send reaction button component (#3997)

* Add Spotlight feature to stateful call client (#3993)

* Add Spotlight feature to stateful call client

* Change files

* Duplicate change files for beta release

* update api

* mark spot light feature as in progress

* added code docs

* Add spotlight feature selectors and handlers (#4004)

* Add Spotlight feature to stateful call client

* Change files

* Duplicate change files for beta release

* update api

* mark spot light feature as in progress

* update api

* Add Spotlight selectors and handlers

* Change files

* Duplicate change files for beta release

* remove ununsed import

* remove duplicate handlers already in common calling handlers

* memoize spotlightedParticipantIds

* remove 'on' prefix from spotlight adapter functions

* fix conditional compilation

* added code docs

* small edit

* Change hidesurvey to disablesurvey based on ARB  (#4006)

* hide survey

* Change files

* Duplicate change files for beta release

---------

Co-authored-by: Donald McEachern <94866715+dmceachernmsft@users.noreply.github.com>

* [Calling Deps] Update to new Calling beta (#4008)

* update to new Calling beta

* Change files

* Duplicate change files for beta release

* cc new type

* fix hermetic

* fix test mock

* Update packages/react-composites ChatComposite browser test snapshots

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Add callback when survey is dismissed for selfhost  (#4009)

* add callback for survey dismissed to selfhost

* Change files

* Duplicate change files for beta release

* build files

* [Calling] Send reaction component calling-component-binding and composite changes (#4003)

* Add license field for package.json files (#4018)

* [Chat] Remove inline image from stable (#4010)

* Revert "[Chat] Remove file card group message from stable API (#3940)"

This reverts commit 74a97ba.

* Revert "[Chat] Rename ImageGallery related Chat Theme (#3936)"

This reverts commit 98b9f7d.

* Revert Inline Image GA changes

* Fix lint issues

* Change files

* Duplicate change files for beta release

* Separate file sharing with inline image

* Remove test CC

* Update FileDownloadCards.test.tsx

* Update ChatScreen.tsx

* Merge strings back into main (#4025)

* TDBuild - updating localized resource files.

* Improve CI workflow (#4016)

* Lock Files

* Change files

* Duplicate change files for beta release

* Update lock files

* Skip compare jest coverage and compare bundle size CI steps for PRs to release (#4026)

* Skip jest test coverage and compare bundle stats steps when base branch is a release branch

* Change files

* Duplicate change files for beta release

* [OCE] Remove beta banner for features that are stable (#4031)

* remove sounds beta banner

* Change files

* Duplicate change files for beta release

* [Calling] Applying capability for Reaction feature (#4013)

* Fix calling sample joining room with room id query arg (#4032)

* Fix calling sample to be able to join room with roomId query arg

* Change files

* Duplicate change files for beta release

* Update packages/react-composites ChatComposite browser test snapshots

* Update packages/react-composites ChatComposite browser test snapshots

---------

Signed-off-by: Donald McEachern <94866715+dmceachernmsft@users.noreply.github.com>
Signed-off-by: Patrick Latter <73612854+palatter@users.noreply.github.com>
Co-authored-by: Donald McEachern <94866715+dmceachernmsft@users.noreply.github.com>
Co-authored-by: Emlyn Bolton <3941071+emlynmac@users.noreply.github.com>
Co-authored-by: James Burnside <2684369+JamesBurnside@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Mohtasim <99507832+Mohtasim@users.noreply.github.com>
Co-authored-by: mgamis-msft <79475487+mgamis-msft@users.noreply.github.com>
Co-authored-by: carocao-msft <96077406+carocao-msft@users.noreply.github.com>
Co-authored-by: vhuseinova-msft <98852890+vhuseinova-msft@users.noreply.github.com>
Co-authored-by: Leah Xia <107075081+Leah-Xia-Microsoft@users.noreply.github.com>
Co-authored-by: edwardlee-msft <edwardlee@microsoft.com>
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.

4 participants