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

replace getAsTag() with getName() on user entity #1016

Merged

Conversation

ankitsmt211
Copy link
Member

replaces User#getAsTag() with User#getName() throughout code, except
RemindRoutine.java, RemindRoutineTest.java(facing some issues in two tests from RemindRoutineTest)
and FilesharingListener.java (already active PR)

@ankitsmt211 ankitsmt211 self-assigned this Jan 26, 2024
@ankitsmt211 ankitsmt211 requested review from a team as code owners January 26, 2024 17:31
@ankitsmt211 ankitsmt211 marked this pull request as draft January 26, 2024 17:38
@ankitsmt211
Copy link
Member Author

@Zabuzard i see you wrote RemindRoutineTest.java

This is causing some trouble and i have no clue, basically replacing getAsTag() in
RemindRoutine.java

with getName() in both files triggers an error MessageEmbed#getAuthor is null so can't call getName() on it.

Perhaps you have some insight ?

@Zabuzard
Copy link
Member

@Zabuzard i see you wrote RemindRoutineTest.java

This is causing some trouble and i have no clue, basically replacing getAsTag() in RemindRoutine.java

with getName() in both files triggers an error MessageEmbed#getAuthor is null so can't call getName() on it.

Perhaps you have some insight ?

Kindly commit and push the change, so I can see the full log with the exact failed test and everything in the CI/CD logs, thank you.

@ankitsmt211
Copy link
Member Author

@Zabuzard these commits contain the problematic changes i was talking about

@Zabuzard
Copy link
Member

@ankitsmt211 the error message is

RemindRoutineTest > Sends out a pending reminder to a guild channel, the base case FAILED
java.lang.NullPointerException at RemindRoutineTest.java:105

RemindRoutineTest > Sends out a pending reminder via DM, even if the channel could not be retrieved anymore FAILED
java.lang.NullPointerException at RemindRoutineTest.java:148

Now, unfortunately I am lacking the full stacktrace. So if you would please run the test manually and send the full stacktrace, that would be awesome 👍

@ankitsmt211
Copy link
Member Author

@ankitsmt211 the error message is

RemindRoutineTest > Sends out a pending reminder to a guild channel, the base case FAILED
java.lang.NullPointerException at RemindRoutineTest.java:105

RemindRoutineTest > Sends out a pending reminder via DM, even if the channel could not be retrieved anymore FAILED
java.lang.NullPointerException at RemindRoutineTest.java:148

Now, unfortunately I am lacking the full stacktrace. So if you would please run the test manually and send the full stacktrace, that would be awesome 👍

@Zabuzard Please have a look

java.lang.NullPointerException: Cannot invoke "okhttp3.OkHttpClient.newCall(okhttp3.Request)" because "this.httpClient" is null
	at net.dv8tion.jda.internal.requests.Requester.execute(Requester.java:180) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.internal.requests.Requester.execute(Requester.java:143) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.internal.requests.Requester.execute(Requester.java:126) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.internal.requests.ratelimit.BotRateLimiter$Bucket.run(BotRateLimiter.java:481) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572) ~[?:?]
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317) ~[?:?]
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?]
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[?:?]
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[?:?]
	at java.base/java.lang.Thread.run(Thread.java:1583) [?:?]
13:40:27.078 [pool-3-thread-1] ERROR net.dv8tion.jda.internal.requests.RateLimiter - Encountered exception trying to execute request
java.lang.NullPointerException: Cannot invoke "java.util.concurrent.ExecutorService.execute(java.lang.Runnable)" because the return value of "net.dv8tion.jda.internal.JDAImpl.getCallbackPool()" is null
	at net.dv8tion.jda.api.requests.Request.onFailure(Request.java:150) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.api.requests.Request.onFailure(Request.java:139) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.internal.requests.RestActionImpl.handleResponse(RestActionImpl.java:284) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.api.requests.Request.handleResponse(Request.java:282) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.internal.requests.Requester.execute(Requester.java:242) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.internal.requests.Requester.execute(Requester.java:143) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.internal.requests.Requester.execute(Requester.java:126) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at net.dv8tion.jda.internal.requests.ratelimit.BotRateLimiter$Bucket.run(BotRateLimiter.java:481) ~[JDA-5.0.0-alpha.20.jar:5.0.0-alpha.20]
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572) ~[?:?]
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317) ~[?:?]
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?]
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[?:?]
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[?:?]
	at java.base/java.lang.Thread.run(Thread.java:1583) [?:?]

java.lang.NullPointerException: Cannot invoke "net.dv8tion.jda.api.entities.MessageEmbed$AuthorInfo.getName()" because the return value of "net.dv8tion.jda.api.entities.MessageEmbed.getAuthor()" is null
	at org.togetherjava.tjbot.features.reminder.RemindRoutineTest.sendsPendingReminderChannelFoundAuthorFound(RemindRoutineTest.java:105)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)```

@Zabuzard
Copy link
Member

Zabuzard commented Feb 1, 2024

@ankitsmt211 thanks, fixed.

Let me quickly explain. getAuthor returned null, why is that the case? It obviously has to do with the change you did in RemindRoutine. So you set a breakpoint there and notice that author.getName() in this line here:

String authorName = author == null ? "Unknown user" : author.getName();

returned null. So authorName = null. This is then set in the embed:

EmbedBuilder().setAuthor(authorName, null, authorIconUrl)

An embed with null as authorName leads to the embed.getAuthor() later in the test to return null consequently.

So now you have to find out why author.getName() was null. Thats because we do not use "real users" here, but mocks. So you go to where this user comes from, namely JdaTester:

UserImpl user = spy(new UserImpl(USER_ID, jda));

Thats the user used on all those messages.
Now, if you look into UserImpl (from JDA), you see its getName() returning a field thats by default null and can be set via setName. So adding that and it works.

In practice, outside of tests, all those fields are populated by the real JDA when receiving Discord API responses etc. But during tests, we have to mock what JDA does ourselves. And this is (obviously) only done to the extend we actually needed. And you just went beyond what was already covered with your change - our mock users had no names yet.

@ankitsmt211
Copy link
Member Author

@ankitsmt211 thanks, fixed.

Let me quickly explain. getAuthor returned null, why is that the case? It obviously has to do with the change you did in RemindRoutine. So you set a breakpoint there and notice that author.getName() in this line here:

String authorName = author == null ? "Unknown user" : author.getName();

returned null. So authorName = null. This is then set in the embed:

EmbedBuilder().setAuthor(authorName, null, authorIconUrl)

An embed with null as authorName leads to the embed.getAuthor() later in the test to return null consequently.

So now you have to find out why author.getName() was null. Thats because we do not use "real users" here, but mocks. So you go to where this user comes from, namely JdaTester:

UserImpl user = spy(new UserImpl(USER_ID, jda));

Thats the user used on all those messages. Now, if you look into UserImpl (from JDA), you see its getName() returning a field thats by default null and can be set via setName. So adding that and it works.

In practice, outside of tests, all those fields are populated by the real JDA when receiving Discord API responses etc. But during tests, we have to mock what JDA does ourselves. And this is (obviously) only done to the extend we actually needed. And you just went beyond what was already covered with your change - our mock users had no names yet.

i think get the idea, by default all mocked values are null unless specified. But i couldn't track down the source where mocked user was setup, thanks for resolving the issue.

@ankitsmt211 ankitsmt211 marked this pull request as ready for review February 1, 2024 19:08
Copy link
Contributor

@marko-radosavljevic marko-radosavljevic left a comment

Choose a reason for hiding this comment

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

This should be merged when the coast is clear. High merge conflict potential. ^^

@Taz03 Taz03 mentioned this pull request Feb 21, 2024
@marko-radosavljevic marko-radosavljevic merged commit 793e7c0 into Together-Java:develop Feb 29, 2024
9 checks passed
Taz03 pushed a commit that referenced this pull request Mar 6, 2024
* replace getAsTag() with getName() on user entity

* problematic changes in RemindRoutine related classes

* addition to problematic changes

* fixed failing tests

---------

Co-authored-by: Zabuzard <[email protected]>
@ankitsmt211 ankitsmt211 mentioned this pull request Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants