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.

Sign up for GitHub

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

Jump to bottom

Add CI for ReSharper.FSharp (Win only) #191

Merged
merged 7 commits into from
Oct 27, 2020
Merged

Add CI for ReSharper.FSharp (Win only) #191

merged 7 commits into from
Oct 27, 2020

Conversation

DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Oct 18, 2020 •
edited
Loading

auduchinok
auduchinok reviewed Oct 18, 2020
View reviewed changes
Copy link
Member

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

It looks very promising. 🙂
I left a few comments, please take a look.

Also the frontend tests run is missing.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
ReSharper.FSharp/test/src/FSharp.Common.Tests/FSharp.Common.Tests.fsproj Outdated
<PackageReference Include="Moq" Version="4.8.2" />
<PackageReference Include="NUnit.Console" Version="3.10.0" />
<PackageReference Include="NUnit3TestAdapter" Version="3.17.0" />
<PackageReference Include="NunitXml.TestLogger" Version="2.1.62" />
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure it doesn't affect local test runs and internal CI.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
ForNeVeR
ForNeVeR approved these changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

Approved in principle, as our glorious leader says.

@citizenmatt
Copy link
Member

It would probably be a good idea to add a Gradle wrapper validation step. This will make sure that any updates to gradle-wrapper.jar are validated against known checksums from the Gradle project, and you know you're not running malicious code (of course, there may be malicious code added to tests, etc. but this is a simple easy check to add)

@DedSec256 DedSec256 changed the base branch from net203 to ber.a/fixForApi October 25, 2020 17:30
@auduchinok
Copy link
Member

auduchinok commented Oct 26, 2020 •
edited
Loading

Looking at the example runs, it shows a step as successful, despite it having some failing tests. Please take a look.
Also, why there's a separate job for showing test results?
Screenshot 2020-10-26 at 13 14 36

@DedSec256 DedSec256 changed the base branch from ber.a/fixForApi to net203 October 26, 2020 16:44
auduchinok
auduchinok approved these changes Oct 26, 2020
View reviewed changes
Copy link
Member

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

@DedSec256 LGTM, thanks! 🙂

@auduchinok auduchinok merged commit 5794840 into JetBrains:net203 Oct 27, 2020
@DedSec256 DedSec256 deleted the ber.a/backend-ci branch January 21, 2021 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@citizenmatt citizenmatt citizenmatt left review comments

@ForNeVeR ForNeVeR ForNeVeR approved these changes

@saul saul saul left review comments

@auduchinok auduchinok auduchinok approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DedSec256 @citizenmatt @auduchinok @saul @ForNeVeR