Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Reflection: Add test verifying ParameterInfo.DefaultValue doesn't throw for optional DateTime parameter#29533

Merged
2 commits merged into
dotnet:masterfrom
stakx:issue-26164
May 10, 2018
Merged

Reflection: Add test verifying ParameterInfo.DefaultValue doesn't throw for optional DateTime parameter#29533
2 commits merged into
dotnet:masterfrom
stakx:issue-26164

Conversation

@stakx

@stakx stakx commented May 5, 2018

Copy link
Copy Markdown
Contributor

This verifies that ParameterInfo.DefaultValue can be queried for optional parameters of type DateTime (and DateTime? just to be sure).

The defined test cases can succeed only with a version of CoreCLR that includes these:

@karelz karelz assigned stakx and ghost May 5, 2018
@stakx stakx changed the title [WIP] Reflection: Add test cases for #26164 [WIP] Reflection: Add test verifying ParameterInfo.DefaultValue doesn't throw for optional DateTime parameter May 5, 2018
@stephentoub

Copy link
Copy Markdown
Member

@dotnet-bot test this please

@stephentoub

Copy link
Copy Markdown
Member

@dotnet-bot test UWP CoreCLR x64 Debug Build please

@ghost

ghost commented May 7, 2018

Copy link
Copy Markdown

Just verified Project N already has this behavior so no extra work needed there.

@ghost

ghost commented May 7, 2018

Copy link
Copy Markdown

This PR should proceed.

(Project N is a separate implementation of the framework for UWP. Changes in Reflection behavior made in CoreCLR often have to be made separately there which is why I went and checked.)

@stakx

stakx commented May 9, 2018

Copy link
Copy Markdown
Contributor Author

@atsushikan - sorry for the newbie question but I have been having a hard time working out why tests are failing. Is this because given this commit, the CI server will pull a previous version of CoreCLR that doesn't have the necessary changes yet?

@stakx stakx changed the title [WIP] Reflection: Add test verifying ParameterInfo.DefaultValue doesn't throw for optional DateTime parameter Reflection: Add test verifying ParameterInfo.DefaultValue doesn't throw for optional DateTime parameter May 9, 2018
@ghost

ghost commented May 9, 2018

Copy link
Copy Markdown

Looks like the CoreCLR update is still being pushed along the side of the road. You should be able to monitor this PR: #29561

@stakx

stakx commented May 10, 2018

Copy link
Copy Markdown
Contributor Author

@dotnet-bot test this please

@ghost

ghost commented May 10, 2018

Copy link
Copy Markdown

not sure how it was passing all jobs (except for only OSX) before?

@stakx

stakx commented May 10, 2018

Copy link
Copy Markdown
Contributor Author

not sure how it was passing all jobs (except for only OSX) before?

@kasper3 - Why, should it not have passed?

(I'm still learning the ropes regarding the whole build & CI setup here, so there's a lot I don't understand yet. There were test failures reported for OSX in the Windows checks, even though the OSX build didn't succeed at all for a probably unrelated reason, so I've retriggered all checks to try to understand whether that's simply some caching problem or an actual issue with this PR.)

@ghost

ghost commented May 10, 2018

Copy link
Copy Markdown

I thought CoreCLR package update PR was merged after @atsushikan's last comment, which means the test validating that change shouldn't pass if that is what it is supposed to validate?

@stakx

stakx commented May 10, 2018

Copy link
Copy Markdown
Contributor Author

@kasper3 - I posted a "test this please" right before the one you're seeing now, which updated the status of most checks to green (they were mostly red before that). I deleted that comment shortly before posting the one you're seeing now because I didn't want to spam this thread with repetitive messages. Another lesson learnt... sorry if this caused confusion.

(PS: And another lesson for me right here... that bot reacts to that phrase even when not explicitly addressed. I have a lot to learn.)

@ghost
ghost merged commit 156cbfa into dotnet:master May 10, 2018
@stakx
stakx deleted the issue-26164 branch May 10, 2018 14:56
@karelz karelz modified the milestones: 1.1.x, 2.2.0 May 12, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…ow for optional DateTime parameter (dotnet/corefx#29533)

* Reflection: Test DefaultValue for DateTime params

* Exclude DateTime test cases for NETFX


Commit migrated from dotnet/corefx@156cbfa
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants