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

Trims output length to avoid hitting github 2Mb limitation #206

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

hraytilakhealthcare
Copy link
Contributor

Changes

  • Truncates the output of output sent to requestGitHubCheck to respect the actual limitation of the platform.

Should fix #142 . Loosely related to #169 .

Discussion

For now the PR only truncates the output without filtering as suggested in #142 and #169. If a test is failed in the said 65534 bytes, the mark will be set to fail. I'm a bit sceptical about the solution, but that's still a improvement over just failing for an unrelated github-limit reason.

Of course, I'm open to other suggestions, such as implementing #169 fully if there is a concensus on it.

I'd gladly get some pointers for doing so, not being familiar with the ecosystem.
Thanks in advance 🙇

Checklist

  • Read the contribution guide and accept the code of conduct
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Cat Gif

@webbertakken
Copy link
Member

Thank you for your contribution.

I'm pondering about whether silently allowing it is indeed better than failing.
I don't know if there are legitimate use cases for logs that are this long - if not then it might in fact be good that it fails.

Curious to hear thoughts from more people.

@timcassell
Copy link
Contributor

timcassell commented Dec 5, 2022

I think this is ok for a temporary solution, but ideally it should be filtered like how dorny/test-reporter does it. Heck, if native NUnit XML ever gets added to that action, it might even be good to integrate it instead of doing it again from scratch.

Also since it's not filtered, the user should at least be notified somehow that they will need to download the full artifact to parse the failures. And I guess they're just out of luck if they haven't configured artifacts already.

[Edit] Or EnricoMi/publish-unit-test-result-action looks like it supports NUnit XML now.

@hraytilakhealthcare
Copy link
Contributor Author

Well I could work on some of these but I have a hard time setting up an environment in which I can test that. Can you advise ?

Copy link
Member

@davidmfinol davidmfinol left a comment

Choose a reason for hiding this comment

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

I think this works as a temporary solution.
I'd still like to see an option to just have a summary without all the details, but that can be done as part of the other issue/with another PR.

@timcassell
Copy link
Contributor

timcassell commented Mar 1, 2023

From #214 this did not fix the issue. Does the dist file need to be re-generated with this change? [Edit] Oh, looks like that was done in #207...

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.

Only 65535 characters are allowed
5 participants