[DISCUSSION] Usage of system properties in tests

[Ivan Bessonov]

Hello Igniters,

I'd like to discuss the way we treat system properties in our test codebase.
It's a common pattern where we set system property in "beforeTestsStarted"
and clear it in "afterTestsStopped". Purest example that I've found is class
"RedisProtocolGetAllAsArrayTest".

There are similar things with "beforeTest"/"afterTest" or huge
"try/finally" blocks
right in test methods.

I think that all this code can be avoided and solution might look like this:

@Test
@WithSystemProperty(key = IGNITE_PROPERTY_NAME, value = "true")
public void testSomething() throws Exception {
    ...
}

Same annotation might be used on class, this way new system property will
be applied to all test methods in the class.

I already created the issue for this change [1] and PR with demo [2]. It
contains
implementation of annotation processing and a few migrated tests. If you
like
the idea then I will migrate all the other tests on the same mechanism.

What do you think?

[1] https://issues.apache.org/jira/browse/IGNITE-11323
[2] https://github.com/apache/ignite/pull/6109

--
Sincerely yours,
Ivan Bessonov

[Dmitry Pavlov]

I find it absolutely positive and modern approach and good contribution.
Count on my support if you will need any assistance with applying this
patch.

чт, 14 февр. 2019 г. в 18:53, Ivan Bessonov <[hidden email]>:

> Hello Igniters,
>
> I'd like to discuss the way we treat system properties in our test
> codebase.
> It's a common pattern where we set system property in "beforeTestsStarted"
> and clear it in "afterTestsStopped". Purest example that I've found is
> class
> "RedisProtocolGetAllAsArrayTest".
>
> There are similar things with "beforeTest"/"afterTest" or huge
> "try/finally" blocks
> right in test methods.
>
> I think that all this code can be avoided and solution might look like
> this:
>
> @Test
> @WithSystemProperty(key = IGNITE_PROPERTY_NAME, value = "true")
> public void testSomething() throws Exception {
>     ...
> }
>
> Same annotation might be used on class, this way new system property will
> be applied to all test methods in the class.
>
> I already created the issue for this change [1] and PR with demo [2]. It
> contains
> implementation of annotation processing and a few migrated tests. If you
> like
> the idea then I will migrate all the other tests on the same mechanism.
>
> What do you think?
>
> [1] https://issues.apache.org/jira/browse/IGNITE-11323
> [2] https://github.com/apache/ignite/pull/6109
>
> --
> Sincerely yours,
> Ivan Bessonov
>

[Dmitriy Govorukhin]

++1 from my side. I guess it will be a more reliable way for works with
properties in the tests. From time to time somebody forgot clear property
after test and next tests may be failed or flaky failed completion.

On Thu, Feb 14, 2019 at 6:56 PM Dmitriy Pavlov <[hidden email]> wrote:

> I find it absolutely positive and modern approach and good contribution.
> Count on my support if you will need any assistance with applying this
> patch.
>
> чт, 14 февр. 2019 г. в 18:53, Ivan Bessonov <[hidden email]>:
>
> > Hello Igniters,
> >
> > I'd like to discuss the way we treat system properties in our test
> > codebase.
> > It's a common pattern where we set system property in
> "beforeTestsStarted"
> > and clear it in "afterTestsStopped". Purest example that I've found is
> > class
> > "RedisProtocolGetAllAsArrayTest".
> >
> > There are similar things with "beforeTest"/"afterTest" or huge
> > "try/finally" blocks
> > right in test methods.
> >
> > I think that all this code can be avoided and solution might look like
> > this:
> >
> > @Test
> > @WithSystemProperty(key = IGNITE_PROPERTY_NAME, value = "true")
> > public void testSomething() throws Exception {
> >     ...
> > }
> >
> > Same annotation might be used on class, this way new system property will
> > be applied to all test methods in the class.
> >
> > I already created the issue for this change [1] and PR with demo [2]. It
> > contains
> > implementation of annotation processing and a few migrated tests. If you
> > like
> > the idea then I will migrate all the other tests on the same mechanism.
> >
> > What do you think?
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-11323
> > [2] https://github.com/apache/ignite/pull/6109
> >
> > --
> > Sincerely yours,
> > Ivan Bessonov
> >
>

[Ivan Bessonov]

Hello Igniters,

current improvement has been merged to master recently,
please feel free to use it in your tests.

пт, 15 февр. 2019 г. в 11:26, Dmitriy Govorukhin <
[hidden email]>:

> ++1 from my side. I guess it will be a more reliable way for works with
> properties in the tests. From time to time somebody forgot clear property
> after test and next tests may be failed or flaky failed completion.
>
> On Thu, Feb 14, 2019 at 6:56 PM Dmitriy Pavlov <[hidden email]> wrote:
>
> > I find it absolutely positive and modern approach and good contribution.
> > Count on my support if you will need any assistance with applying this
> > patch.
> >
> > чт, 14 февр. 2019 г. в 18:53, Ivan Bessonov <[hidden email]>:
> >
> > > Hello Igniters,
> > >
> > > I'd like to discuss the way we treat system properties in our test
> > > codebase.
> > > It's a common pattern where we set system property in
> > "beforeTestsStarted"
> > > and clear it in "afterTestsStopped". Purest example that I've found is
> > > class
> > > "RedisProtocolGetAllAsArrayTest".
> > >
> > > There are similar things with "beforeTest"/"afterTest" or huge
> > > "try/finally" blocks
> > > right in test methods.
> > >
> > > I think that all this code can be avoided and solution might look like
> > > this:
> > >
> > > @Test
> > > @WithSystemProperty(key = IGNITE_PROPERTY_NAME, value = "true")
> > > public void testSomething() throws Exception {
> > >     ...
> > > }
> > >
> > > Same annotation might be used on class, this way new system property
> will
> > > be applied to all test methods in the class.
> > >
> > > I already created the issue for this change [1] and PR with demo [2].
> It
> > > contains
> > > implementation of annotation processing and a few migrated tests. If
> you
> > > like
> > > the idea then I will migrate all the other tests on the same mechanism.
> > >
> > > What do you think?
> > >
> > > [1] https://issues.apache.org/jira/browse/IGNITE-11323
> > > [2] https://github.com/apache/ignite/pull/6109
> > >
> > > --
> > > Sincerely yours,
> > > Ivan Bessonov
> > >
> >
>

--
Sincerely yours,
Ivan Bessonov