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

JAMES-3140 Add a per server testing builder #3531

Closed

Conversation

chibenwa
Copy link
Member

@chibenwa chibenwa commented Jul 1, 2020

It hides the generic typing complexity of the JamesServerBuilder
and addresses testing configuration duplication while still exposing
module combination configuration in every test.

@chibenwa chibenwa added the refactoring Code enhencements that don't alter software behaviour label Jul 1, 2020
@chibenwa chibenwa added this to the Sprint - common - 1 milestone Jul 1, 2020
@chibenwa chibenwa added the WIP Work In Progress. Not ready for review, possibly low code quality. label Jul 1, 2020
@chibenwa chibenwa marked this pull request as draft July 1, 2020 04:43
@chibenwa chibenwa self-assigned this Jul 1, 2020
It hides the generic typing complexity of the JamesServerBuilder
and addresses testing configuration duplication while still exposing
module combination configuration in every test.
@chibenwa chibenwa force-pushed the JAMES-3140-testing-server-builder branch from 88524b8 to da18061 Compare July 1, 2020 04:55
@chibenwa chibenwa removed the WIP Work In Progress. Not ready for review, possibly low code quality. label Jul 1, 2020
Copy link

@rouazana rouazana left a comment

Choose a reason for hiding this comment

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

seems nice

@chibenwa chibenwa marked this pull request as ready for review July 1, 2020 07:24
@chibenwa
Copy link
Member Author

chibenwa commented Jul 1, 2020

MemorySpamAssassinContractTest.imapMovesToSpamMailboxShouldBeConsideredAsSpam » ConditionTimeout

test this please

@chibenwa chibenwa linked an issue Jul 1, 2020 that may be closed by this pull request
Copy link

@mbaechler mbaechler left a comment

Choose a reason for hiding this comment

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

I fail to see what's the point of this PR. It looks like a builder call extraction (anti-pattern ?) dealing with a single type of Server.
What do we gain?

@chibenwa
Copy link
Member Author

chibenwa commented Jul 2, 2020

[da18061a9736f5e08949686b163f8a404a5584dc] [ERROR]   MailboxesRoutesTest$FixingReIndexing$TaskDetails.userReprocessingShouldReturnTaskDetailsWhenFailingAtTheMailboxLevel:1304 1 expectation failed.
[da18061a9736f5e08949686b163f8a404a5584dc] JSON path status doesn't match.
[da18061a9736f5e08949686b163f8a404a5584dc] Expected: is "failed"
[da18061a9736f5e08949686b163f8a404a5584dc]   Actual: null

Fixed elsewhere.

test this please

@chibenwa
Copy link
Member Author

chibenwa commented Jul 2, 2020

I fail to see what's the point of this PR. It looks like a builder call extraction (anti-pattern ?) dealing with a single type of Server.
What do we gain?

#3531 (comment) explains this.

The team tend to like this proposal.

@mbaechler
Copy link

I fail to see what's the point of this PR. It looks like a builder call extraction (anti-pattern ?) dealing with a single type of Server.
What do we gain?

#3531 (comment) explains this.

The team tend to like this proposal.

Ok, let's ask more precise questions

It hides the generic typing complexity of the JamesServerBuilder

What typing complexity? In this PR it's always used with a single type, right?

and addresses testing configuration duplication while still exposing module combination configuration in every test.

I understand it puts in common these 3 lines :

                .workingDirectory(tmpDir)
                .configurationFromClasspath())
                .build());

does it do more that than?

@chibenwa
Copy link
Member Author

chibenwa commented Jul 2, 2020

does it do more that than?

It turns:

new JamesServerBuilder<CassandraRabbitMQJamesConfiguration>(...)

Into

TestingDistributedJamesServerBuilder.

Which is simpler.

As we might get more options in the future, we could add staged builders to make writing integration test easier.

@mbaechler
Copy link

does it do more that than?

It turns:

new JamesServerBuilder<CassandraRabbitMQJamesConfiguration>(...)

Into

TestingDistributedJamesServerBuilder.

Which is simpler.

Are you arguing List is simpler than List<Foo>? Or that ListInt is simpler than List<Integer>?

Sorry, I don't buy that.

As we might get more options in the future, we could add staged builders to make writing integration test easier.

What about just making the real server builder good enough that we don't need another layer to abstract it?

@chibenwa
Copy link
Member Author

chibenwa commented Jul 6, 2020

What about just making the real server builder good enough that we don't need another layer to abstract it?

Sounds hard.

The server builder (generic) don't know which configuration builder to be using (specific)

Suggestions?

@mbaechler
Copy link

What about just making the real server builder good enough that we don't need another layer to abstract it?

Sounds hard.

The server builder (generic) don't know which configuration builder to be using (specific)

Suggestions?

Nested builders.

DistributedJames.builder().generic(builder -> builder.foo().bar()).specific1().build();

@chibenwa
Copy link
Member Author

chibenwa commented Jul 6, 2020

Nested builders.

Sorry, we end up with something specific anyway, I don't see how it differ from what is proposed here.

@chibenwa
Copy link
Member Author

See #3568

@chibenwa chibenwa closed this Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code enhencements that don't alter software behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distributed James: Disable ElasticSearch
6 participants