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.

Sign up for GitHub

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

Jump to bottom

Implemented IJSInProcessRuntime and IJSUnmarshalledRuntime interf… #279

Merged
merged 8 commits into from
Dec 12, 2020
Merged

Implemented IJSInProcessRuntime and IJSUnmarshalledRuntime interf… #279

merged 8 commits into from
Dec 12, 2020

Conversation

KristofferStrube
Copy link
Contributor

@KristofferStrube KristofferStrube commented Dec 6, 2020 •
edited by egil
Loading

Pull request description

Pull request for issue #222
I use #IF NET5_0 in the middle of namespaces/classes to differentiate what can be implemented in the different versions. This might not follow the AspNetCore coding guidelines.

PR meta checklist

  • Pull request is targeting at DEV branch.
  • I have read the CONTRIBUTING.md document.
  • Pull request is linked to all related issues, if any.

Content checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

@codecov
Copy link

codecov bot commented Dec 6, 2020 •
edited
Loading

Codecov Report

Merging #279 (1e6915a) into dev (8235d93) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #279      +/-   ##
==========================================
+ Coverage   82.61%   82.64%   +0.02%     
==========================================
  Files         118      118              
  Lines        3607     3612       +5     
  Branches      468      468              
==========================================
+ Hits         2980     2985       +5     
  Misses        480      480              
  Partials      147      147
Impacted Files Coverage Δ
src/bunit.web/JSInterop/BunitJSInterop.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b83bd3f...1e6915a. Read the comment docs.

egil
egil requested changes Dec 6, 2020
View reviewed changes
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Looks pretty good I think. Besides a few missing "happy path tests" in strict mode, I think you are almost there.

src/bunit.web/JSInterop/BunitJSInterop.cs Outdated Show resolved Hide resolved
src/bunit.web/JSInterop/BunitJSInterop.cs Outdated Show resolved Hide resolved
@KristofferStrube
Removed DisallowNull attribute at InvokeUnmarshalled's arguments …
44c5766 
…and parsed an empty array instead of null to `InvokeUnmarshalled` with no arguments.
egil
egil reviewed Dec 7, 2020
View reviewed changes
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Latest changes looks good to me.

@KristofferStrube
Copy link
Contributor Author

I will just add some more tests for the "happy path tests" later today. Do you think it's ready for non-draft Pull-Request after that?

@KristofferStrube KristofferStrube marked this pull request as ready for review December 7, 2020 14:54
egil
egil requested changes Dec 8, 2020
View reviewed changes
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Looks good. Only seem to be missing some happy path IJSUnmarshalledRuntime tests.

tests/bunit.web.tests/JSInterop/BunitJSInteropTest.cs Outdated Show resolved Hide resolved
@egil
Copy link
Member

egil commented Dec 8, 2020

I unchecked the My change requires a change to the documentation. since I do not think this is necessary. The way we have set this up should result in a "it just works" experience for users. The task #261 should cover a general description of this feature.

@egil egil mentioned this pull request Dec 8, 2020
Closed
5 tasks
egil
egil reviewed Dec 9, 2020
View reviewed changes
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@egil
Copy link
Member

egil commented Dec 9, 2020

I will just add some more tests for the "happy path tests" later today. Do you think it's ready for non-draft Pull-Request after that?

It was ready to begin with 🙂

Just missing the happy path tests, then we can close this.

egil
egil reviewed Dec 9, 2020
View reviewed changes
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

There are a few code style/code cleanup tidbits here, but otherwise I think we are done.

tests/bunit.web.tests/JSInterop/BunitJSInteropTest.net5.cs Outdated Show resolved Hide resolved
tests/bunit.web.tests/JSInterop/BunitJSInteropTest.net5.cs Outdated Show resolved Hide resolved
egil
egil approved these changes Dec 12, 2020
View reviewed changes
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks.

@egil egil merged commit 16b149d into bUnit-dev:dev Dec 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egil egil egil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KristofferStrube @egil