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

Add comments to binding gen tests for invocations on new lines #91237

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Aug 28, 2023

Addresses #91218 (comment).

@layomia layomia added this to the 8.0.0 milestone Aug 28, 2023
@layomia layomia self-assigned this Aug 28, 2023
@ghost
Copy link

ghost commented Aug 28, 2023

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Addresses #91218 (comment).

Author: layomia
Assignees: layomia
Labels:

area-Extensions-Configuration

Milestone: 8.0.0

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 28, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 29, 2023
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

I added an extra suggestion, LGTM otherwise. Thanks!

[Fact]
public void TestBindingInvocationsWithIrregularCSharpSyntax()
{
// Tests binding invocation variants with irregular C# syntax, interspersed with white space.
Copy link
Member

Choose a reason for hiding this comment

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

Each statement through here should get the same treatment of a comment for each different variation of syntax. @tarekgh -- Or should we make each scenario its own [Fact] to make it even more clear?

Copy link
Member

Choose a reason for hiding this comment

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

I think having separate Fact method for each case will be too much especially if we'll need to add more in the future. The most important thing for me is to ensure that anyone touching this code will get a chance to see the attached comment to that line of code and understand why we wrote it this way.

@jeffhandley
Copy link
Member

@layomia / @tarekgh -- I pushed a couple commits that expand on the comments and reorganize the tests a bit. If this looks good to you, I'll also approve and we can merge.

The spirit of what I changed:

  1. Separate out the API scenarios into their own tests
  2. Within each test, organize and comment on the newline scenarios covered
  3. Add newline scenarios for "Newlines in every possible place"
  4. In the tests with multiple asserts, do not reuse record instances across the asserts (to guard against a hidden failure).
  5. Refactor the Assert method out and have it perform a single Assert on a Tuple so that both values are shown together (so that if the assert fails, the message will be clearer)

@layomia In the ConfigurationExtensions tests, there were no Asserts in the test. Are asserts needed, or does a non-throwing execution indicate success?

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

@jeffhandley changes LGTM. Thanks!

@tarekgh tarekgh dismissed jeffhandley’s stale review August 30, 2023 16:19

Jeff already submitted the commits that address his comments.

@layomia
Copy link
Contributor Author

layomia commented Aug 30, 2023

@layomia In the ConfigurationExtensions tests, there were no Asserts in the test. Are asserts needed, or does a non-throwing execution indicate success?

Indeed non-throwing execution indicates success. The tests are really testing successful compilation. Input syntax doesn't affect the emitted binding logic, which is being tested by the other functional tests.

@layomia
Copy link
Contributor Author

layomia commented Aug 30, 2023

Thanks Jeff & Tarek for the detailed review; and Jeff thanks for helping to improve the comments.

@layomia layomia merged commit a16670a into dotnet:main Aug 30, 2023
105 checks passed
layomia added a commit that referenced this pull request Aug 30, 2023
* Add comments to binding gen tests for invocations on new lines

* Address feedback & test static method call syntax

* Reorganize and comment the newline/whitespace scenarios

* Reorganize and comment the newline/whitespace scenarios for ConfigurationExtensions

---------

Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
carlossanlop pushed a commit that referenced this pull request Aug 30, 2023
… on separate line (#91218)

* Emit interceptor info correctly when invocation expr is on separate line

* Add more tests and add helper to udpate baselines

* Add comments to binding gen tests for invocations on new lines (#91237)

* Add comments to binding gen tests for invocations on new lines

* Address feedback & test static method call syntax

* Reorganize and comment the newline/whitespace scenarios

* Reorganize and comment the newline/whitespace scenarios for ConfigurationExtensions

---------

Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>

* Update baselines

---------

Co-authored-by: Layomi Akinrinade <laakinri@microsoft.com>
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
@ghost ghost locked as resolved and limited conversation to collaborators Sep 29, 2023
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