这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@sazzer
Copy link
Contributor

@sazzer sazzer commented Mar 19, 2025

No description provided.

}

@Test
@Disabled // Needs credentials configuring
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we specify how somewhere, maybe in a comment at the top of the file, so if a reader checks out the repo they understand what to do?


import static org.junit.jupiter.api.Assertions.assertEquals;

public class ClientUnitTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is making a live connection it should be a LiveTest so it doesn't make the CI build flaky on connection issues


import static org.junit.jupiter.api.Assertions.assertEquals;

public class UsersUnitTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is making a live connection it should be a LiveTest so it doesn't make the CI build flaky on connection issues


GHUser user = gitHub.getUser("eugenp");
List<GHRepository> repositoriesList = user.listRepositories().toList();
assertEquals(6, repositoriesList.size());
Copy link
Collaborator

Choose a reason for hiding this comment

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

These kind of assertions will not be stable (can't rely on it always being 6)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better to just change this to a log then? Or is it ok to have unstable assertions if I change it to a LiveTest?

String branchHash = branch.getSHA1();

GHCommit commit = repository.getCommit(branchHash);
System.out.println(commit.getCommitShortInfo().getMessage());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have guidlines not to use System.out.println - use an SLF4J logger instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I even knew that and still forgot to do it. :( Fixing.

Comment on lines 20 to 22
assertEquals("someone", myself.getLogin());
assertEquals("someone@example.com", myself.getEmail());
assertEquals(50, myself.getFollows().size());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Confused about this could ever have passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wouldn't. This was just an example. It's not really possible to have stable assertions for this test, since it relies on knowing which user is authenticating.
I can just remove it if that's preferred? Or would it be better to just change the assertions to logs?

sazzer and others added 5 commits March 22, 2025 15:41
Co-authored-by: Liam Williams <liam.williams@zoho.com>
Co-authored-by: Liam Williams <liam.williams@zoho.com>
….java

Co-authored-by: Liam Williams <liam.williams@zoho.com>
….java

Co-authored-by: Liam Williams <liam.williams@zoho.com>

GHUser user = gitHub.getUser("eugenp");
List<GHRepository> repositoriesList = user.listRepositories().toList();
assertTrue(repositoriesList.size() > 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have the convention/guideline to use AssertJ for assertions like this, e.g.assertThat(..).isNotEmpty()

@theangrydev theangrydev merged commit 18fb857 into eugenp:master Mar 28, 2025
1 check passed
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.

2 participants