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

Fix/naming convention jsinterop #150

Merged
merged 17 commits into from
Jun 19, 2020
Merged

Fix/naming convention jsinterop #150

merged 17 commits into from
Jun 19, 2020

Conversation

yourilima
Copy link
Contributor

@yourilima yourilima commented Jun 15, 2020 •
edited
Loading

Pull request description

PR meta checklist

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

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.

Change Checklist

  • Rename Js to JS in Class, Method and File names
  • Update documentation to reflect changes
  • Update sample project to reflect changes in naming
  • Move mocks to the TestDoubles folder

@yourilima
Remove files
ce00c01 
In preparation to refactor of the files
@yourilima
Refactor names
372d8a9
@yourilima yourilima changed the base branch from master to dev June 15, 2020 08:41
@yourilima
Copy link
Contributor Author

yourilima commented Jun 15, 2020 •
edited
Loading

For Issue #147 ( I cant seem to link Issue and PR)

Requires a bit of updating the docs.

I am in doubt if this requires an update in changelog. Probably due to changes to naming being a breaking change. Am i correct in this?

@yourilima
Updated README.md with suggestion.
036a8a9 
Suggestion how the change could sound.
@yourilima
Copy link
Contributor Author

I added a suggestion for a change in the readme.

@egil
Copy link
Member

egil commented Jun 15, 2020

@yourilima yes, update the changelog in the "Unreleased beta-8" -> "Changed" section. Write something with same structure as the existing entry in that section.

@egil egil self-requested a review June 15, 2020 09:40
egil
egil requested changes Jun 15, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@egil egil linked an issue Jun 15, 2020 that may be closed by this pull request
Make sure JSMock uses right casing, e.g. JsRuntimeMock to JSRuntimeMock #147
Closed
@egil
Copy link
Member

egil commented Jun 15, 2020

I know it is a bit tedious, but if you can also:

  1. Update the /sample/tests/* tests that use JSInterop. You might have to update /sample/tests/SampleApp.Tests.csproj with project references to make it work locally, like /docs/sapmles/tests/xunit/bunit.docs.xunit.samples.csproj.
  2. Update the /docs/site/docs/csharp-test-examples.md examples with the new syntax. That will make it easier for me when I have to write the https://bunit.egilhansen.com/docs/mocking/mocking-ijsruntime page.

@yourilima
Copy link
Contributor Author

I know it is a bit tedious, but if you can also:

Don't worry about the tediousness :)

  1. Update the /sample/tests/* tests that use JSInterop. You might have to update /sample/tests/SampleApp.Tests.csproj with project references to make it work locally, like /docs/sapmles/tests/xunit/bunit.docs.xunit.samples.csproj.

Should be done i ran tests locally and everything is running as it should. i suppose i should not update the projects dependency yet? or should i for the sake of the entire repo to be able to run and test? Then when you finalize beta-8 you'll update the reference to <PackageReference Include="bunit" Version="1.0.0-beta-8" />, or should i do that now prematurely?

  1. Update the /docs/site/docs/csharp-test-examples.md examples with the new syntax. That will make it easier for me when I have to write the https://bunit.egilhansen.com/docs/mocking/mocking-ijsruntime page.

changed /docs/docs/csharp-test-examples.md as i presume you moved it in master

@yourilima yourilima requested a review from egil June 15, 2020 21:30
@egil
Copy link
Member

egil commented Jun 16, 2020

Then when you finalize beta-8 you'll update the reference to <PackageReference Include="bunit" Version="1.0.0-beta-8" />, or should i do that now prematurely?

Leave it at 1.0.0-beta-8 now, thats fine.

changed /docs/docs/csharp-test-examples.md as i presume you moved it in master

Correct. I need to merge master into dev ASAP. Thanks for the heads up.

egil
egil requested changes Jun 16, 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. There is one more thing we should change now that we have these files open, its the namespace. A related issue @DarthPedro is working on (#146) is adding a new folder and namespace called TestDoubles, which is a more general term for mocks/stubs/fakes/dummys, so lets move MockJSRuntime into that as well. I.e.:

  • Move all the JSMock code from /Mocking to /TestDoubles, in both the bunit.web and bunit.web.tests projects.
  • Move all types that are in the Bunit.Mocking.JSInterop to be in the Bunit.TestDoubles.JSInterop namespace.

CHANGELOG.md Outdated Show resolved Hide resolved
@egil
Copy link
Member

egil commented Jun 16, 2020

Sorry, messed you up a little bit by merging main into dev, so the file in docs have moved.

@yourilima yourilima marked this pull request as draft June 16, 2020 11:45
@yourilima
Copy link
Contributor Author

Don't worry I can fix it

@egil
Copy link
Member

egil commented Jun 17, 2020

FYI: I have updated the dev branch with test-doubles as the name for the documentation section.

@egil egil added this to the beta-8 milestone Jun 17, 2020
@yourilima yourilima marked this pull request as ready for review June 19, 2020 11:00
@yourilima yourilima requested a review from egil June 19, 2020 11:04
@egil
Copy link
Member

egil commented Jun 19, 2020

Thanks @yourilima. It looks like all the boxes are ticked.

Do you consider this PR ready to be merged?

@yourilima
Copy link
Contributor Author

yourilima commented Jun 19, 2020 •
edited
Loading

Do you consider this PR ready to be merged?

I do. the checkbox for documentation only relates to /docs/site/docs/csharp-test-examples.md as you mentioned you'd update the docs separately. If that is correct I think it is ready.

This is a breaking change though. How will that be reflected in the changelog?

@yourilima
Copy link
Contributor Author

The move to TestDoubles folder is a change to be attributed to @DarthPedro so i omitted that on the changelog for now.

@egil
Copy link
Member

egil commented Jun 19, 2020

The move to TestDoubles folder is a change to be attributed to @DarthPedro so i omitted that on the changelog for now.

He is doing work on another PR which is attributed to him, so I think it is fine.

I will merge then. Thanks for your help. Let me know if you want to help out with other issues.

@egil egil merged commit c2cf2b3 into bUnit-dev:dev Jun 19, 2020
@yourilima yourilima deleted the fix/naming-convention-jsinterop branch June 22, 2020 21:02
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

@yourilima yourilima

Labels
None yet
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.

Make sure JSMock uses right casing, e.g. JsRuntimeMock to JSRuntimeMock
2 participants
@yourilima @egil