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

Updated mono testing doc #44360

Merged
merged 23 commits into from
Nov 20, 2020
Merged

Updated mono testing doc #44360

merged 23 commits into from
Nov 20, 2020

Conversation

fanyang-mono
Copy link
Member

No description provided.

@ghost
Copy link

ghost commented Nov 6, 2020

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

I believe we already have a file for running tests on Mono at https://github.com/dotnet/runtime/blob/master/docs/workflow/testing/mono/testing.md

We should be cleaning up/extending this instead.

@fanyang-mono
Copy link
Member Author

fanyang-mono commented Nov 6, 2020

I believe we already have a file for running tests on Mono at https://github.com/dotnet/runtime/blob/master/docs/workflow/testing/mono/testing.md

We should be cleaning up/extending this instead.

The file name is a little bit misleading. This file is for running runtime tests only. I have another PR for that which refers to this file, feel free to review that as well.
#44362

@CoffeeFlux
Copy link
Contributor

Is there a reason we're moving some of those instructions over to the CoreCLR directory? It seems better suited in the original document.

@fanyang-mono
Copy link
Member Author

Is there a reason we're moving some of those instructions over to the CoreCLR directory? It seems better suited in the original document.

Follow the fashion as this doc:
https://github.com/dotnet/runtime/blob/master/docs/workflow/testing/libraries/testing-android.md and https://github.com/dotnet/runtime/blob/master/docs/workflow/testing/libraries/testing-wasm.md

@CoffeeFlux
Copy link
Contributor

I think that right now we're better off avoiding splitting mono instructions into a directory that's otherwise CoreCLR-specific (and out of date). It seems confusing and unlikely to be found there. If we restructure the docs to have a runtime tests folder (as opposed to a CoreCLR one) I think it would make more sense, but otherwise I'd prefer to preserve the current structure.

@fanyang-mono
Copy link
Member Author

fanyang-mono commented Nov 6, 2020

I think that right now we're better off avoiding splitting mono instructions into a directory that's otherwise CoreCLR-specific (and out of date). It seems confusing and unlikely to be found there. If we restructure the docs to have a runtime tests folder (as opposed to a CoreCLR one) I think it would make more sense, but otherwise I'd prefer to preserve the current structure.

It makes more sense to keep the detailed instructions of running runtime tests with mono together with other runtime tests running instructions. Because there are other details about the runtime tests which are not covered by this doc and could easily be discovered, if they all live in the same folder.

@CoffeeFlux
Copy link
Contributor

CoffeeFlux commented Nov 6, 2020

I think the primary use of the docs is people looking for quick instructions on "how to do [x]", and this doesn't strike me as conducive to that. Again, I'd be fine with a larger restructuring to group everything by test suite, but I'd rather avoid mixing approaches since that makes docs unnecessarily difficult to find.

At the very least, it would be nice to get some other opinions before merging a structural change like this, so I'm going to leave it blocked for now.

I'd also prefer to group that other PR with this one since they're dependent on each other; I don't think it makes sense to review or merge them separately.

@SamMonoRT
Copy link
Member

@fanyang-mono - For libraries, since now they are common for both the runtimes, it makes sense the way they have it. For Mono and CoreCLR, it makes sense to have individual how-tos documentation in their relative folder structures. It makes more sense to leave Mono instructions in the mono dir. You should update the name of the file 'testing.md' to 'testing-mono.md' and update that file contents. You can close this PR, and use the other PR #44362 to make those changes.

@steveisok - It makes sense to extract the testing libraries section from here to under the libraries documentation. Any recommendations ?

@fanyang-mono
Copy link
Member Author

I feel that there maybe a misunderstanding here, which probably was caused by creating two separated PR's for my doc changes. That was the only option which was the only option was given to me when editing online. I will find a way to merge my 2 PR's.

This file is NOT a replacement of the original testing doc for mono. This file only contains content regarding to runtime tests. That file still exist and have a direct link to this new file. So this won't make discovering mono documents more difficult.

Again, the reason why I put this file here is because other files in this folder covers quite a few details about the nature of runtime tests. The runtime tests are now being shared by coreclr and mono. Maybe this folder name could be changed from coreclr to runtime.

That's being said, I am okay with merging the content here to mono/testing.md, if this setup is confusing to most people. My motivation was to make it easy to find information that you need.

@fanyang-mono fanyang-mono changed the title Create testing-mono.md Updated mono testing doc Nov 6, 2020
@fanyang-mono
Copy link
Member Author

Closed #44362 and kept this one, since all the discussions were done here.

@fanyang-mono
Copy link
Member Author

Created another issue (#44433) to centralize the document for runtime tests.

Copy link
Contributor

@naricc naricc 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 to me as far as the actual documentation goes. I am fine with leaving it in mono/testing.md and merging the two testing document dirs as a seperate issue.

@fanyang-mono
Copy link
Member Author

@CoffeeFlux Are you okay with this change now?

Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

I went through a bunch of restructuring specifics in the PR, but for some broader feedback I recommend using more concise and direct wording when writing technical documentation. Some of my suggested edits reflect that, but there are more places where that could be cleaned up.

docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
@fanyang-mono
Copy link
Member Author

@CoffeeFlux Any other feedback?

Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Getting there! Few more stylistic notes, some comments on using more direct and inclusive language, and then a suggestion on how to restructure the samples section.

docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Show resolved Hide resolved
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Couple specific things left, but structurally I'm happy with where it's at now. Should be good once you fix those.

docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved
docs/workflow/testing/mono/testing.md Outdated Show resolved Hide resolved

3. Run the tests
* To run the desktop Mono sample, cd to `HelloWorld` and execute:
Copy link
Contributor

Choose a reason for hiding this comment

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

The bullet points are back :(
Please use sub-headers.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are very short. I thought bullet points were more suitable.

Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fanyang-mono fanyang-mono merged commit a6b3b23 into master Nov 20, 2020
@jkotas jkotas deleted the fanyang-mono-patch-1 branch November 21, 2020 02:56
@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2020
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.

6 participants