Skip to content

Conversation

@psfinaki
Copy link
Contributor

@psfinaki psfinaki commented Feb 3, 2023

The tests for now are in C#. That's because:

  1. the integration testing framework API is subject to change
  2. the current API is very F#-unfriendly
  3. we are still investigating this topic

Tests run in a separate CI leg:
image

image

@psfinaki psfinaki linked an issue Feb 6, 2023 that may be closed by this pull request
@psfinaki psfinaki marked this pull request as ready for review February 10, 2023 20:10
@psfinaki psfinaki requested a review from a team as a code owner February 10, 2023 20:10
@psfinaki psfinaki changed the title [WIP] Integration tests for F# Integration tests for F# Feb 10, 2023
@psfinaki psfinaki requested a review from sharwell February 10, 2023 20:11
@T-Gro
Copy link
Member

T-Gro commented Feb 10, 2023

This is from the CI run on this PR:

Passed! - Failed: 0, Passed: 7, Skipped: 0, Total: 7, Duration: 7 m 17 s - FSharp.Editor.IntegrationTests.dll (net472)

I don't think this is a good idea to have.
As of now, I am not convinced that the benefits for this repo outweigh the added time and added reason for failing CI.

T-Gro
T-Gro previously requested changes Feb 10, 2023
Copy link
Member

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

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

Passed! - Failed: 0, Passed: 7, Skipped: 0, Total: 7, Duration: 7 m 17 s - FSharp.Editor.IntegrationTests.dll (net472)

@psfinaki
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Contributor Author

@T-Gro everything is an "added reason for failing CI". If these proves to be consistently passing, this shouldn't be an obstacle in that sense.

Extra time is a different thing. A few minutes is probably something we can live with but yeah that's not scalable. I will look into what other repos do about integration tests in CI, we should basically follow. @shamwell any advice?

@psfinaki
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki psfinaki enabled auto-merge (squash) February 14, 2023 13:37
@vzarytovskii vzarytovskii dismissed T-Gro’s stale review February 15, 2023 11:38

Addressed most, let's run it in CI for a while and see if it's stable enough for us.

@psfinaki psfinaki merged commit 3c80121 into dotnet:main Feb 15, 2023
@psfinaki psfinaki deleted the psfinaki/integration-tests branch February 15, 2023 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Investigate integration tests

4 participants