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.

Sign up for GitHub

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

Jump to bottom

JAMES-2586 Integration tests for JMAP postgres #2029

Merged
merged 2 commits into from
Mar 4, 2024
Merged

JAMES-2586 Integration tests for JMAP postgres #2029

merged 2 commits into from
Mar 4, 2024

Conversation

hungphan227
Copy link
Contributor

@hungphan227 hungphan227 commented Feb 20, 2024 •
edited

I currently disable some failed tests because they are not easy to fix (need to check business logic). I will create other tickets to fix these tests

@hungphan227 hungphan227 mentioned this pull request Feb 20, 2024
Closed
@chibenwa
Copy link
Contributor

Do annotate @disabled tests in a separate commit to ease review please!

@hungphan227 hungphan227 force-pushed the JAMES-2586-Integration-tests-for-JMAP-postgres branch from e7d93e6 to f141a39 Compare February 21, 2024 02:50
Arsnael
Arsnael approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@Arsnael Arsnael left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can you create a ticket though for fixing those unstable tests? Then I'm ok to merge it :)

vttranlina
vttranlina reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@vttranlina vttranlina left a comment

Choose a reason for hiding this comment

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

Some file configs were copied from the memory package,
I think we can try with postgres

backends-common/postgres/src/test/java/org/apache/james/backends/postgres/PostgresFixture.java Outdated Show resolved Hide resolved
...integration-tests/postgres-jmap-rfc-8621-integration-tests/src/test/resources/lmtpserver.xml Outdated Show resolved Hide resolved
...ration-tests/postgres-jmap-rfc-8621-integration-tests/src/test/resources/mailetcontainer.xml Outdated Show resolved Hide resolved
...on-tests/postgres-jmap-rfc-8621-integration-tests/src/test/resources/mailrepositorystore.xml Outdated Show resolved Hide resolved
...ration-tests/postgres-jmap-rfc-8621-integration-tests/src/test/resources/usersrepository.xml Outdated Show resolved Hide resolved
@hungphan227 hungphan227 force-pushed the JAMES-2586-Integration-tests-for-JMAP-postgres branch from f141a39 to fdb033c Compare February 21, 2024 11:04
@hungphan227
Copy link
Contributor Author

Looks good to me. Can you create a ticket though for fixing those unstable tests? Then I'm ok to merge it :)

linagora#5077

@Arsnael
Copy link
Contributor

Arsnael commented Feb 23, 2024

Issues maybe? => https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2029/5/

04:37:01.592 [INFO ] o.a.j.b.p.DockerPostgresSingleton - 2024-02-23 04:37:01.591 UTC [75] ERROR:  relation "event_dead_letters" does not exist at character 13

@hungphan227
Copy link
Contributor Author

Issues maybe? => https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2029/5/

04:37:01.592 [INFO ] o.a.j.b.p.DockerPostgresSingleton - 2024-02-23 04:37:01.591 UTC [75] ERROR:  relation "event_dead_letters" does not exist at character 13

:(( It still run fine in my local. Does any other PR have the same issue?

@Arsnael
Copy link
Contributor

Arsnael commented Feb 26, 2024 •
edited

Potentially racing issue? Like I had with domain table before?

@Arsnael
Copy link
Contributor

Arsnael commented Feb 26, 2024

No sorry didnt realize that the faulty tests in question were not even part of this PR. Thinking more of a bad test isolation here..?

@Arsnael Arsnael force-pushed the JAMES-2586-Integration-tests-for-JMAP-postgres branch 2 times, most recently from 9c8398e to e055627 Compare February 26, 2024 07:46
@Arsnael
Copy link
Contributor

Arsnael commented Feb 27, 2024 •
edited

I think should do it... the rabbitmq.properties file that you added @hungphan227 created issues. Because of it it was thinking of using rabbitmq for all tests classes in server/apps/postgres-app. There was an injection issue (user and password missing for uri management)

Also not all tests are using rabbitmq.

Rabbitmq values are being injected into the conf in the tests that need it btw, so unnecessary changes.

Please avoid making changes that are not related to the work, here integration tests, the test resources on the server-app module are not related to this work

quantranhong1999
quantranhong1999 reviewed Feb 28, 2024
View reviewed changes
...ends-common/postgres/src/test/java/org/apache/james/backends/postgres/PostgresExtension.java
Comment on lines +182 to +184
if (!rlsEnabled) {
dropAllConnections();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Another way without introducing more code: move disposePostgresSession from afterAll to afterEach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what disposePostgresSession close is connection of PostgresExtension, not connections created when running PostgresJamesServerMain

@Arsnael
Copy link
Contributor

Arsnael commented Feb 28, 2024

@hungphan227 related? https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2029/15/

Arsnael
Arsnael reviewed Feb 28, 2024
View reviewed changes
...s/src/test/java/org/apache/james/jmap/rfc8621/distributed/DistributedAuthenticationTest.java Outdated
@@ -52,6 +52,5 @@ class DistributedAuthenticationTest implements AuthenticationContract {
.extension(new AwsS3BlobStoreExtension())
.server(configuration -> CassandraRabbitMQJamesServerMain.createServer(configuration)
.overrideWith(new TestJMAPServerModule()))
.lifeCycle(JamesServerExtension.Lifecycle.PER_ENCLOSING_CLASS)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not keen on having modifications on other tests suites than Postgres tbh on a separate working branch than master

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree with @Arsnael. We should not modify the contract test unless something is actually wrong with the contract test itself.

@hungphan227 maybe try more on releasing the connection upon James shutdown? As I think the production code deserves the connection release as well.
e.g. add this to the SinglePostgresConnectionFactory

    @PreDestroy
    public void dispose() {
        Mono.from(connection.close()).block();
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@quantranhong1999 you commented on wrong thread

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'm not keen on having modifications on other tests suites than Postgres tbh on a separate working branch than master

I have reversed code and used a new solution

@hungphan227
Copy link
Contributor Author

81e7747 has been reversed by 02981dd

@hungphan227 hungphan227 force-pushed the JAMES-2586-Integration-tests-for-JMAP-postgres branch from 02981dd to 37851b5 Compare February 29, 2024 10:53
@hungphan227
Copy link
Contributor Author

Disabled PostgresAuthenticationTest due to JamesServerExtension. Created another ticket: linagora#5099

@Arsnael Arsnael merged commit bbf0ef8 into apache:postgresql Mar 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arsnael Arsnael Arsnael approved these changes

@quantranhong1999 quantranhong1999 quantranhong1999 approved these changes

@vttranlina vttranlina vttranlina left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@hungphan227 @chibenwa @Arsnael @quantranhong1999 @vttranlina