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

Add "Guardian" test anti-pattern #358

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add "Guardian" test anti-pattern #358

wants to merge 2 commits into from

Conversation

danon
Copy link
Contributor

@danon danon commented May 22, 2019

Let me know If you approve another anti-pattern? :)

@0crat
Copy link

0crat commented May 22, 2019

@yegor256/z please, pay attention to this pull request

@0crat
Copy link

0crat commented May 22, 2019

@danon this pull request is too small, just 4 lines changed (less than 10), there will be no formal code review, see §53 and §28; in the future, try to make sure your pull requests are not too small; @yegor256/z please review this and merge or reject

Copy link
Contributor

@johndotpage johndotpage left a comment

Choose a reason for hiding this comment

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

Typo: "dangerous" not "dangrous".

@johndotpage
Copy link
Contributor

Sorry. Still working out how this "review" stuff works... :)

@johndotpage
Copy link
Contributor

@danon Have you got a clearer example of this? As far as asserting that something doesn't happen is concerned, it seems perfectly fine to assert that something doesn't throw, for example.

Co-Authored-By: John <50244516+johndotpage@users.noreply.github.com>
@danon
Copy link
Contributor Author

danon commented May 22, 2019

What I mean is

// given
void shouldNotAppendExtensionForBlocked() {
  // given
  Resource r = new Resource("/content/docs/file.txt", "some word");
  r.makeWrittable(false);
  Creator c = new Creator();

  // when
  c.append("file.txt", "");

  // then
  assertEquals(r.get(), "some word");
}

How are you going to check if this test is not a Liar? Normally, you would comment the when section or remove logic from the Creator class, but in this case - anything you do won't make it fail.

Second question: how can you be sure your assertions are correct?

  • Maybe the class is valid and your code doesn't do what it wasn't supposed to do
  • Maybe the class is invalid and it does something what it wasn't supposed to, but assertions didn't pick it up

@johndotpage
Copy link
Contributor

johndotpage commented May 23, 2019

@danon I'm not sure I follow (maybe I've missed the point of the code). If the desired behaviour is that appending an empty string doesn't change the file at all, then that test has some value. Not a lot, sure. But some. (It's just an example after all, right?)

I would probably rewrite the test (and the code), but it would still be a specification of desired behaviour.

void appending_an_empty_string_to_a_text_file_does_not_change_the_files_contents() {
  Text file = new Text("/content/docs/file.txt", "some text");

  file.append("");

  assertEquals(file.contents(), "some text");
}

And now, if appending an empty string added "empty string was appended" or else threw an exception, then this test would catch it.

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.

None yet

3 participants