Skip to content

Add unit testing for commands, JDA mock system #182

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

Merged
merged 5 commits into from
Oct 21, 2021

Conversation

Zabuzard
Copy link
Member

@Zabuzard Zabuzard commented Oct 12, 2021

Overview

Closes #152. Adds a unit testing framework for commands revolving around JDA mocks with Mockito.

Original proposal by @Heatmanofurioso, optimized and adjusted by me. (still quite some TODOs in there though, can be done in other PRs).

Also adds high unit coverage for PingCommand and DatabaseCommand.

Example

Basic usage example:

SlashCommand command = new DatabaseCommand(database);
JdaTester jdaTester = new JdaTester();

SlashCommandEvent event = jdaTester.createSlashCommandEvent(command)
  .subcommand("get")
  .option("key", "foo")
  .build();
command.onSlashCommand(event);

verify(event, times(1)).reply("Saved message: bar");

Details

It revolves around the class JdaTester which is basically a mock of JDA and friends. The class offers a createSlashCommandEvent method that, using a builder-pattern, enables users to create their own command events. Those events can then be given to command.onSlashCommand(...) to trigger the test logic. Testing is then done via Mockito, since the events are Mockito mocks.

The builder provides strong subcommand and option validation using the CommandData settings from command.getData(). That way, users will get runtime exceptions when trying to create wrong events, such as:

SlashCommandEvent event = jdaTester.createSlashCommandEvent(new PingCommand())
  .subcommand("foo")
  .build();
// throws because "PingCommand" has no subcommand called "foo"

The builder has quite some background bloat because unfortunately JDA does not provide utility to create events. The core of its logic is creating a DataObject with DataObject.fromJson(json). The JSON has to be in the format specified by the Discord API (see the actual payloads send by Discord), which we construct over a POJO bridge using Jackson.

Mockito

Note that the approach of Mockito mocking JDA is quite fragile. It is highly likely that adding any new command that uses some other features, will run into a JDA routine that has not been covered by the mock yet - causing NPEs and other annoyance. That means that anyone who wants to test their commands usually also has to spend some time on expanding the JDA mock.

Also, major updates in the internal API of JDA will likely break the mock, we are using their internal code (i.e. their ...Impl classes) directly here.

I do not see any nice way around that though - it is what it is.

@Zabuzard Zabuzard added enhancement New feature or request priority: major labels Oct 12, 2021
@Zabuzard Zabuzard added this to the Initial Setup milestone Oct 12, 2021
@Zabuzard Zabuzard self-assigned this Oct 12, 2021
@Zabuzard Zabuzard requested review from a team as code owners October 12, 2021 17:03
@Zabuzard
Copy link
Member Author

CodeFactor only complains about the // TODOs. They can be addressed in another PR though.

Tais993
Tais993 previously approved these changes Oct 12, 2021
Copy link
Member

@Tais993 Tais993 left a comment

Choose a reason for hiding this comment

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

This does make the current project a lot harder, creating commands just got a whole lot more complicated, so not sure how that's gonna go with less knowledged programmers

@Zabuzard
Copy link
Member Author

Zabuzard commented Oct 12, 2021

This does make the current project a lot harder, creating commands just got a whole lot more complicated, so not sure how that's gonna go with less knowledged programmers

@Tais993
Testing commands is not a requirement. It is an option.
I am open for better alternatives but dropping optional command testing all together doesnt sound better to me.

The major problem with testing is that we expose JDA and its API to the user. We do not hide it behind our own API and hence also do not have control over it. Which is why Mockito seems to be the only option. (To be clear, I am not voting for hiding JDA, its just the reason why testing gets so nasty)

One advantage of this proposal is that writing the actual test is still simple, from which beginners will benefit. Maintaining the platform behind it is the obstacle, which experienced devs can take responsibility for - I do get your point though and you are right.

Tais993
Tais993 previously approved these changes Oct 13, 2021
@Zabuzard Zabuzard mentioned this pull request Oct 14, 2021
Heatmanofurioso and others added 5 commits October 14, 2021 17:23
less confusing, clearer, gets rid of name clash
* Use shared pools instead of creating new instances each time
* weaken type of pools
* spotless
@Zabuzard
Copy link
Member Author

History cleanup and added @Heatmanofurioso back into the history

@Zabuzard Zabuzard enabled auto-merge (rebase) October 14, 2021 15:25
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Zabuzard Zabuzard disabled auto-merge October 21, 2021 10:20
@Zabuzard Zabuzard merged commit 672608c into develop Oct 21, 2021
@Zabuzard Zabuzard deleted the feature/add_jda_mock branch October 21, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Unit tests for commands
3 participants