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

feat: added contents for testing of wasmedge apps #177

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

adithyaakrishna
Copy link
Contributor

Description:

This PR adds the written content for testing of WasmEdge applications

Copy link
Collaborator

alabulei1 commented Oct 21, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall Summary:

The pull request titled "feat: added contents for testing of wasmedge apps" includes several individual summaries of the key changes made. After considering all the individual summaries, it is clear that there are a few potential issues and errors that need attention.

The first potential problem is the deletion of the test.md file without a clear explanation. The reason for this deletion should be clarified with the author.

Next, there is a typo in the code snippet in the testing-types.md file. The incomplete code snippet should be completed or fixed.

Furthermore, the new content added for testing wasmedge apps lacks context, which makes it difficult to assess the correctness and completeness of the changes.

In addition to these potential issues, there are a few important findings worth mentioning. The changes include updates to the lists of tools and frameworks used for testing wasmedge applications. However, the commit message should provide more context and explanation about why the lists were updated. Furthermore, a new link for testing wasmedge has been added, and the file best-practises copy.md has been renamed to best-practises.md. The reviewer should ensure that the new link is correctly formatted and provides relevant information for testing wasmedge, and also confirm the reason for the file rename.

Overall, the changes appear to add new testing documentation for wasmedge apps. However, there are potential issues and errors that need clarification, and additional context and explanation is required for a thorough assessment of the changes.

Details

Commit 5f89bd23e2da1d4013bdba33f2ae935e73029f23

Key changes:

  • Added new files related to testing of wasmedge apps: _category_.json, best-practises copy.md, intro.md, testing-types.md.
  • Modified existing file test.md by deleting its contents.

Potential problems:

  • It is unclear why the file test.md was deleted. This should be clarified with the author.
  • The file testing-types.md has a typo in the last line (EXPECT_EQ(Result.value().g). The code snippet should be completed or fixed.
  • The changes seem to add new content for testing wasmedge apps, but without further context, it is difficult to assess the correctness and completeness of the changes.

Overall, the changes appear to add new testing documentation for wasmedge apps. However, the reasons for deleting the test.md file should be clarified, and the incomplete code snippet in testing-types.md should be fixed.

Commit 490b64a51ffdb1cf9b13abdc5f20369899f6f0ac

Key changes:

  • Updated the lists of tools and frameworks used for testing WasmEdge applications.
  • Reordered the list items.

Potential problems:

  • The commit message should provide more context and explanation about why the lists were updated.

Commit d5b9ad3596a3f2cdd637e949d997f98e6d1dba73

Key changes:

  • The patch adds a new link for testing WasmEdge in the docs/contribute/overview.md file.

Potential problems:

  • The link for testing WasmEdge has been changed from test.md to ./testing/intro.md. The reviewer should ensure that the new link points to the correct file and that the file exists in the proper location.
  • Since this is a documentation change, there may not be any functional impact. However, the reviewer should still confirm that the new link is correctly formatted and provides relevant information for testing WasmEdge.

Commit 72404a1323888a80093355dbde0be5c22327ea1f

Key changes in the patch:

  1. The patch adds Chinese (zh) documentation for testing WasmEdge applications.
  2. The test.md file is deleted and replaced by a new intro.md file.
  3. A new _category_.json file is created for the "Testing" category.
  4. A new best-practises copy.md file is created in the testing category.

Potential problems:

  1. The file test.md is deleted, but it is not clear why it is removed. The reason for this deletion should be explained in the pull request or commit message.
  2. The file intro.md is added, but it is not clear what is the purpose of this file and how it relates to the rest of the documentation. The pull request should provide more context and explanation for this addition.

Overall, the changes seem to be adding Chinese documentation for testing WasmEdge applications. However, more information and context is needed to fully understand the purpose and impact of these changes.

Commit 7cbe458793407f9dc6f83d84524c57f9aa7cf4f1

Key Changes:

  • Renamed the file "best-practises copy.md" to "best-practises.md" in both the "docs/contribute/testing" and "i18n/zh/docusaurus-plugin-content-docs/current/contribute/testing" directories.

Potential Problems:

  • No code changes or updates are included in this patch. It only reflects a file rename.
  • It is unclear why the file was renamed and if it has any impact on the functionality or content.
  • The commit message does not provide enough context or explanation for the file rename.

Suggested Improvements:

  • It would be helpful to provide a clear reason for the file rename in the commit message.
  • If there are changes or updates to the file content, those should be included in this patch as well.

Commit 2c4cef1fb69e69e527a4a4d7443c26a353a5a165

Key changes:

  • The title of the document has been updated from "Best Practises" to "Best Practises for WasmEdge Application Testing".
  • The changes have been made in two files: docs/contribute/testing/best-practises.md and i18n/zh/docusaurus-plugin-content-docs/current/contribute/testing/best-practises.md.

Potential problems:

  • There doesn't seem to be any potential problems with these changes. The changes made are relatively minor and appear to be just updating the title of the document.

"position": 4,
"link": {
"type": "generated-index",
"description": "We will learn how to test WasmEdge applications"
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the contributor guide, I think we should learn how to test WasmEdge itself, not the WasmEdge applications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alabulei1 As discussed, how abt if we move this so that users can know how to test wasmedge apps?

If so, could you please suggest a subheading where I can move this to?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Testing Wasm apps is the question that the Wasm community wants to solve. I don't think we can solve the problem.

docs/contribute/testing/best-practises copy.md Outdated Show resolved Hide resolved
docs/contribute/testing/intro.md Show resolved Hide resolved
Signed-off-by: Adithya Krishna <[email protected]>
"position": 4,
"link": {
"type": "generated-index",
"description": "We will learn how to test WasmEdge applications"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Testing Wasm apps is the question that the Wasm community wants to solve. I don't think we can solve the problem.

docs/contribute/testing/best-practises.md Outdated Show resolved Hide resolved
Signed-off-by: Adithya Krishna <[email protected]>
Copy link
Member

@hydai hydai left a comment

Choose a reason for hiding this comment

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

I have no idea why this is called WasmEdge applications?
If you are developing the WasmEdge internal, tools, and plugins, then it should be WasmEdge testing.

"position": 4,
"link": {
"type": "generated-index",
"description": "We will learn how to test WasmEdge applications"
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect. The testing should be test WasmEdge itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hydai This is mostly for the contributors, we have different testing docs for WasmEdge itself. Ref PR: #197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants