Skip to content

Conversation

@pblasucci
Copy link
Contributor

@pblasucci pblasucci commented May 3, 2025

This PR slightly modifies the code in TestExplorer.fs, such that if a TRX file contains output, it is emitted for passing tests. This is similar to the behavior of JetBrains Rider. Further, it facilitates having meaningful output when, eg, using FsCheck.

For example, the following test:

[<Property>]
let ``test some stuff`` (PositiveInt value) =
    (0 < value) |> Prop.collect value

will print output like the following:
image

Happy to hear feedback on this (esp. from @farlee2121 and @TheAngryByrd). Thanks!

@farlee2121
Copy link
Contributor

farlee2121 commented May 4, 2025

I think that's a great feature add!

I left one comment on the intended structure of the TRX types.
Someone else might have thoughts about the global.json and package.json changes.

Some related work: I'm working on moving us away from TRX parsing to using the VsTest platform and eventually the Microsoft Testing Platform.

I did some poking around to make sure we'll be able to support StdOut once the move from TRX to VsTest or MTP is done.
MTP clearly supports standard out per test, but I haven't been able to track down how it'd work with VsTest. It's got to be possible if the info is available in the trx file.

@pblasucci
Copy link
Contributor Author

I think that's a great feature add!

Thanks! Happy to help. I've change the code per your suggestions.

Someone else might have thoughts about the global.json and package.json changes.

For context, I needed the change to global.json in order to get things to build (without my having to go hunt down a specific older version of the SDK). However, I have no idea, really, about the packages.json file -- it's just something the tooling automatically changed for me (I really do not understand all the Fable / JS / TS wizardry).

@TheAngryByrd
Copy link
Member

global.json

It's such a pain, it's always wrong 😂 . Don't worry about it.

package.json

looks to be pinning a version of yarn, which is probably fine.

Copy link
Member

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

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

Gonna approve it. @pblasucci have any other changes before I merge and ship it?

@pblasucci
Copy link
Contributor Author

Nope. I'm all set @TheAngryByrd. If it's good for you and @farlee2121, then it's good for me. 👍

@TheAngryByrd TheAngryByrd merged commit c9bcf37 into ionide:main May 5, 2025
2 checks passed
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