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

Regression tests for comparers #577

Closed
wants to merge 4 commits into from
Closed

Regression tests for comparers #577

wants to merge 4 commits into from

Conversation

manofstick
Copy link
Contributor

This is a set of regression tests for #513. It's is branched from the point just before modifications in that pull requests began.

Possibly this is a bit of overkill, but I suppose that is the point of regression tests. This will be merged into #513, but is here to obviously prove that it was OK prior to the changes.

The data is mainly generated by running the CreateComparersRegression.fsx and pasting the results over the top of the bottom part of the ComparersRegression.fs file (I suppose it could have been a seperate file... Maybe that would have been better; but I'm hoping there really isn't much need to run it multiple times)

@msftclas
Copy link

msftclas commented Aug 9, 2015

Hi @manofstick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@manofstick
Copy link
Contributor Author

@latkin,

Argh, the compiler is inlining these (obviously) and so no really achieving what I want. I will have to regenerate. Soon, soon....

@latkin
Copy link
Contributor

latkin commented Aug 11, 2015

+47,167 -0

shades

This is actually right in line with what I think we need - a near-exhaustive set of regression tests that really nail down how things work.

Looking at the CI build, this doesn't appear to have too drastic of an effect on compilation or execution time, which I was a bit worried about.

@manofstick
Copy link
Contributor Author

OK; hopefully this is a better test suite.

I have now ensured that there are both the potential for the compiler to do inlining optimizations as well as not. This has meant that the number of tests has doubled.

But, I have shrunk the format a little (limiting spaces, using 0,1 for false and true) and so the actual file is a little smaller than it was before.

@manofstick
Copy link
Contributor Author

I'm feeling too lazy at the moment (and it's not really in scope of what I'm trying to achieve), but slight modifications to this suite could be used to pick up any inconsistencies between compiler optimizations and not, as well as between direct versions and wrapped versions, embedded in other types, etc...

i.e. for #576, #527 & #556

(on another note, when those items are fixed, then obviously this regression suite would need to be updated)

@latkin
Copy link
Contributor

latkin commented Aug 13, 2015

I'd like to merge this separately, and before, #513. That way it's unambiguous that behavior remains unchanged before/after 513 is applied.

@latkin latkin added this to the F# 4.0 Update 1 milestone Aug 13, 2015
@manofstick
Copy link
Contributor Author

@latkin

I agree, that was why I created this pull request :-)

(wow, it's almost like we're talking in real time!)

@latkin
Copy link
Contributor

latkin commented Aug 13, 2015

Ah ok, "this will be merged into 513" made me think you planned to keep them together.

@manofstick
Copy link
Contributor Author

No, I just merged it in so that I could make sure it worked before and after.

@manofstick
Copy link
Contributor Author

(like it has been merged in, but I still expected the regression tests to run in a unmolested state...)

Hmmm.. that make sense?

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