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

Unit test fixes #17

Closed
wants to merge 2 commits into from
Closed

Unit test fixes #17

wants to merge 2 commits into from

Conversation

mdietz198
Copy link

I ran pitest and found two things:

  1. An order dependency in the WebsocketControllerTest due to the static fields
  2. Some assertThat lines that were not actually asserting anything

Using the pitest results, I added some more assertions and tests - mostly for error cases - in a branch. If that help is appreciated, I can keep adding some more coverage and make another pull request. If not, no worries. If you're more interested in contributions to the client, I'd be happy to give that a shot.

@moxie0
Copy link
Contributor

moxie0 commented Jan 10, 2017

sorry I didn't see this before, looks like it needs a rebase

The former assertThat(value == 4) was not actually performing an
assertion since there was no .isEqualTo or .isTrue()
@mdietz198
Copy link
Author

Looks like the first commit is no longer relevant since WebsocketControllerTest is gone. The other two commits still apply.

@moxie0
Copy link
Contributor

moxie0 commented Jan 30, 2017

sorry, it also looks like you haven't signed the CLA

@odeke-em
Copy link

odeke-em commented Jun 4, 2017

Ping @mdietz198 -- after signing the CLA, perhaps a squash of the commits as well.

@posix4e posix4e mentioned this pull request Jun 24, 2017
@posix4e
Copy link
Contributor

posix4e commented Jun 29, 2017

Honestly I think this should be closed in favour of #81

visitpay pushed a commit to visitpay/Signal-Server that referenced this pull request Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants