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

Add information about writing tests #80

Closed
wants to merge 8 commits into from
Closed

Add information about writing tests #80

wants to merge 8 commits into from

Conversation

ruishanteo
Copy link

Fixes #79

mfjkri
mfjkri reviewed Aug 22, 2024
View reviewed changes
Copy link
Contributor

@mfjkri mfjkri left a comment

Choose a reason for hiding this comment

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

Hi Rui Shan! I believe this will be a great addition, as it allows students to follow along and learn about the various aspects of the code they need to write tests for as they go. I just have a few more additions for the tests based on the Git diffs in the tutorial.

tutorials/ab3AddRemark.md Outdated
- Since we created a new file RemarkCommand, we should also create a `RemarkCommandTest` file. JUnit tests are annotated with `@Test`. For now, we expect `execute` in `RemarkCommand` to fail. Thus, the test will reflect this expectation.

- ```java
assertCommandFailure(new RemarkCommand(), model, MESSAGE_NOT_IMPLEMENTED_YET);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we include the full test instead? For clarity and consistency

@Test
 public void execute() {
     assertCommandFailure(new RemarkCommand(), model, MESSAGE_NOT_IMPLEMENTED_YET);
 }

Copy link
Author

Choose a reason for hiding this comment

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

Okay, done!

tutorials/ab3AddRemark.md Outdated
@@ -141,6 +165,47 @@ public class RemarkCommand extends Command {
}
```

<panel header="Update construction of `RemarkCommand` in `RemarkCommandTest`">

- Since `RemarkCommand`’s constructor now accepts arguments, the tests will reflect that.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since both the constructor and execute method of RemarkCommand was updated, how about phrasing this like this instead?

Since RemarkCommand now requires both an index and a remark for instantiation, the tests should be updated accordingly. Additionally, the execution of the command has been updated, so the tests should be adjusted to reflect these changes.

@Test
public void execute() {
   final String remark = "Some remark";

   assertCommandFailure(new RemarkCommand(INDEX_FIRST_PERSON, remark), model,
           String.format(MESSAGE_ARGUMENTS, INDEX_FIRST_PERSON.getOneBased(), remark));
}

Copy link
Author

Choose a reason for hiding this comment

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

Amended it according to this too!

@Carlintyj
Copy link

Hello Rui Shan,

Great job on the proposed changes! I’ve reviewed your work, and I have a few additional suggestions that might help enhance the clarity and usefulness of the tutorial for the readers:

  1. Explanation of @test Annotation: Perhaps you could consider including a brief explanation of what the @Test annotation does when you first mention it. This will help readers who may not be as familiar with JUnit understand its purpose and how it fits into the testing process.

  2. Context for Test-Writing: At the beginning of each test-writing section, it might be helpful to add a sentence or two explaining why the tests are being written at that point in the tutorial. This provides context and highlights the importance of testing in the development workflow.

  3. Link to JUnit Resources: Including a link to the official JUnit documentation or a relevant tutorial could be beneficial for readers who want to explore unit testing in more depth.

Thank you for your hard work and besides these, LGTM!

@damithc
Copy link
Contributor

damithc commented Aug 26, 2024

  1. Explanation of @test Annotation: Perhaps you could consider including a brief explanation of what the @Test annotation does when you first mention it. This will help readers who may not be as familiar with JUnit understand its purpose and how it fits into the testing process.

@Carlintyj I'm not sure if we need this one. At this point students should have used JUnit in the iP already. We can point to the JUnit guide but we can't keep repeating all the basics in every tutorial :-)

@Carlintyj
Copy link

Okay, sounds good to me to just point to the JUnit guide. 😃 Thanks prof @damithc

@ruishanteo
Copy link
Author

ruishanteo commented Aug 27, 2024 •
edited
Loading

@Carlintyj

  1. Context for Test-Writing: At the beginning of each test-writing section, it might be helpful to add a sentence or two explaining why the tests are being written at that point in the tutorial. This provides context and highlights the importance of testing in the development workflow.

Not sure what this refers to exactly. I have included short sentences/ clauses to explain why the tests are being written. For example: Since RemarkCommand now requires both an index and a remark for instantiation, the tests should be updated accordingly. Additionally, the execution of the command has been updated, so the tests should be adjusted to reflect these changes. Could you explain further/ give some examples on what this point refers to? Thanks!

  1. Link to JUnit Resources: Including a link to the official JUnit documentation or a relevant tutorial could be beneficial for readers who want to explore unit testing in more depth.

Done! Added a link in the first panel.

@Carlintyj
Copy link

Since we created a new file RemarkCommand, we should also create a RemarkCommandTest file. JUnit tests are annotated with @test. For now, we expect execute in RemarkCommand to fail. Thus, the test will reflect this expectation.

I was thinking instead of saying we should also create RemarkCommandTest file as such, we could perhaps just drop a sentence or two to give further clarification. For example: "it’s crucial to ensure that our code behaves as expected. This is why we write tests at each step of the process. These tests not only verify that the new functionality works correctly but also help prevent future changes from inadvertently breaking the code."

Hope this clarifies my intentions.

@ruishanteo
Copy link
Author

@Carlintyj
I see! I added that explanation to the main panel explaining that tests are written. I think that the line given was general so I added it to the introductory panel instead of the individual panels where tests are explained.

jyue487
jyue487 reviewed Aug 28, 2024
View reviewed changes
Copy link

@jyue487 jyue487 left a comment

Choose a reason for hiding this comment

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

Overall, LGTM! But are some of the contents overlapping with the Test section in IP?

tutorials/ab3AddRemark.md Outdated
- ```java
@Test
public void execute() {
final String remark = "Some remark";
Copy link

Choose a reason for hiding this comment

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

I am not too sure but should this be indented?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Added the indentation.

damithc pushed a commit that referenced this pull request Sep 8, 2024
ab3AddRemarkd: Add more info on testing
a4cf2b5 
#80

Main author: @ruishanteo
@damithc
Copy link
Contributor

damithc commented Sep 8, 2024

@ruishanteo Thanks for this PR.
I incorporated your changes in a4cf2b5

@damithc damithc closed this Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyue487 jyue487 jyue487 left review comments

@mfjkri mfjkri mfjkri left review comments

Assignees
No one assigned
Labels
status.ForReview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[AB3: Adding Command] Test writing instructions
5 participants
@ruishanteo @Carlintyj @damithc @mfjkri @jyue487