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

feat!: use streams to refactor writeResultFiles #352

Merged
merged 8 commits into from
Mar 28, 2024

Conversation

peternhale
Copy link
Collaborator

I modifies the testService.writeFiles to use streams to produce the requested output formats on disk.

This is necessary to handle very large test results that were causing string length violations when running
JSON.stringify for json results.

@W-15135036@

AC:
Should be no changes to results produced by tests.

@peternhale peternhale marked this pull request as ready for review March 11, 2024 17:24
}
);

this.push('\n\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

probably a dumb q, but does this cover all OS newlines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll fix this

Copy link
Contributor

@gbockus-sf gbockus-sf left a comment

Choose a reason for hiding this comment

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

This make sense.
Two suggestions

  • Ensure we do a major version bump when this is merged
  • Probably would be good to define test plan that covers both the small and large responses and the different code flows we need to cover.

gbockus-sf

This comment was marked as duplicate.

@peternhale peternhale changed the title feat: use streams to refactor writeResultFiles feat!: use streams to refactor writeResultFiles Mar 22, 2024
Copy link
Member

@mingxuanzhangsfdx mingxuanzhangsfdx left a comment

Choose a reason for hiding this comment

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

I tested it from plugin-apex and vsce. Looks good to me! Thanks a lot for the change!

@peternhale peternhale merged commit 9c2123d into main Mar 28, 2024
11 checks passed
@peternhale peternhale deleted the phale/W-15135036-streams branch March 28, 2024 17:57
peternhale added a commit that referenced this pull request Apr 17, 2024
This reverts commit 9c2123d.

# Conflicts:
#	src/streaming/codeCoverageStringifyStream.ts
#	src/streaming/streamingClient.ts
#	src/streaming/testResultStringifyStream.ts
#	src/tests/asyncTests.ts
#	test/streaming/jsonStringifyStream.test.ts
gilgourevitch pushed a commit to gilgourevitch/salesforcedx-apex that referenced this pull request Jun 6, 2024
* chore: wip

* feat: refactor results to use streams

@W-15135036@
refactor to use streams while producing results after tests are complete

* chore: add human report transform and stream

* chore: cleanup after merging main

* refactor: create narrowing functions

* chore: add elapsed time decorator to streamingClient

* chore: replace hardcoded newlines with os.EOL
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