[yast-devel] Change in yast-rake and fragile tests

newer
[yast-devel] Barclays - Important...

older
[yast-devel] Pending Pull Requests

Ancor Gonzalez Sosa

15 Jan 2015 15 Jan '15

16:53

This change introduced in our test:unit task has caused the tests in yast-yast2 to break. https://github.com/yast/yast-rake/commit/b00ca0e8c8208b05698675b0cb4752552de... It's actually not a fault of the change in the task. The problem is that the tests are fragile because they pollute the global namespace in several ways. To be honest, I'd expect test from some other repositories to also break. Specially those written when we were still newbies with RSpec. I did some changes in order to fix it but found another problem. Here is the patch, not intended to enhance or refactor the tests in any way, just to fix the minimum needed to make them run again. https://github.com/ancorgs/yast-yast2/commit/a5c09480081be09d894f2df3a8b36a3... The weird thing is that I needed to introduce the line marked with a FIXME, which means that closing the default SCR is not as secure as all our tests assume. Looking at the yast2-core source code I'd say that the line is actually needed and we need to update quite some tests and the documentation. But it's the core, so of course I could be wrong. Cheers. -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Show replies by date

Ladislav Slezak

16 Jan 16 Jan

08:43

Dne 15.1.2015 v 17:53 Ancor Gonzalez Sosa napsal(a):

...

This change introduced in our test:unit task has caused the tests in yast-yast2 to break. https://github.com/yast/yast-rake/commit/b00ca0e8c8208b05698675b0cb4752552de...

That commit runs all tests in a single process, the advantages are: - faster run (almost by factor 10!) - proper code coverage (before the change every run rewrote the previous code coverage statistics, at the end the report contained only the coverage from the last run) See https://github.com/yast/yast-rake/issues/11 for details.

...

It's actually not a fault of the change in the task. The problem is that the tests are fragile because they pollute the global namespace in several ways.

Ouch, I didn't realize this side effect... [...]

...

The weird thing is that I needed to introduce the line marked with a FIXME, which means that closing the default SCR is not as secure as all our tests assume. Looking at the yast2-core source code I'd say that the line is actually needed and we need to update quite some tests and the documentation. But it's the core, so of course I could be wrong.

I have checked the code, and yes, after closing the current SCR you need to set the current one as no default SCR is set: https://github.com/yast/yast-core/blob/master/wfm/src/Y2WFMComponent.cc#L301 But I'm not much familiar with that part of Yast... -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak@suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Arvin Schnell

08:50

On Fri, Jan 16, 2015 at 09:43:27AM +0100, Ladislav Slezak wrote:

...

That commit runs all tests in a single process, the advantages are:

- faster run (almost by factor 10!)

- proper code coverage (before the change every run rewrote the previous code coverage statistics, at the end the report contained only the coverage from the last run)

If I have tests in several subdirs do I get the correct coverage? Regards, Arvin -- Arvin Schnell, Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Josef Reidinger

10:09

On Fri, 16 Jan 2015 09:50:46 +0100 Arvin Schnell wrote:

...

On Fri, Jan 16, 2015 at 09:43:27AM +0100, Ladislav Slezak wrote:

...
That commit runs all tests in a single process, the advantages are:

- faster run (almost by factor 10!)

- proper code coverage (before the change every run rewrote the previous code coverage statistics, at the end the report contained only the coverage from the last run)

If I have tests in several subdirs do I get the correct coverage?

Regards, Arvin

It is possible, but there is one tricky part, that for correct coverage you need to require all modules and libs ( otherwise you get coverage only of files that are used in any test). I plan to work on it for yast2 as part of https://trello.com/c/WSN9JR4v/234-clean-up-of-yast-yast2 Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Ladislav Slezak

14:24

Dne 16.1.2015 v 09:50 Arvin Schnell napsal(a):

...

On Fri, Jan 16, 2015 at 09:43:27AM +0100, Ladislav Slezak wrote:

...
That commit runs all tests in a single process, the advantages are:

- faster run (almost by factor 10!)

- proper code coverage (before the change every run rewrote the previous code coverage statistics, at the end the report contained only the coverage from the last run)

If I have tests in several subdirs do I get the correct coverage?

Yes, it should work correctly. The only problem is that it obviously works only with the new RSpec tests, the legacy tests will be ignored in the code coverage. (That's another reason to get rid of them...) And as Josef pointed out the code coverage evaluates only *loaded* files, so if you have a file which does not have any test (and is never loaded from the other tests) then it won't be taken into account when computing total code coverage percentage. You can get false high test coverage... As a workaround you can force loading all files, like here in the registration module: https://github.com/yast/yast-registration/blob/master/test/spec_helper.rb#L3... -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak@suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Josef Reidinger

14:50

On Fri, 16 Jan 2015 15:24:55 +0100 Ladislav Slezak wrote:

...

Dne 16.1.2015 v 09:50 Arvin Schnell napsal(a):

...
On Fri, Jan 16, 2015 at 09:43:27AM +0100, Ladislav Slezak wrote:

...
That commit runs all tests in a single process, the advantages are:

- faster run (almost by factor 10!)

- proper code coverage (before the change every run rewrote the previous code coverage statistics, at the end the report contained only the coverage from the last run)

If I have tests in several subdirs do I get the correct coverage?

Yes, it should work correctly.

The only problem is that it obviously works only with the new RSpec tests, the legacy tests will be ignored in the code coverage. (That's another reason to get rid of them...)

And as Josef pointed out the code coverage evaluates only *loaded* files, so if you have a file which does not have any test (and is never loaded from the other tests) then it won't be taken into account when computing total code coverage percentage. You can get false high test coverage...

As a workaround you can force loading all files, like here in the registration module: https://github.com/yast/yast-registration/blob/master/test/spec_helper.rb#L3...

It do not load modules, so I think better one is from bootloader as bootloader uses old modules: https://github.com/yast/yast-bootloader/blob/master/test/test_helper.rb#L11 Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Josef Reidinger

10:14

On Fri, 16 Jan 2015 09:43:27 +0100 Ladislav Slezak wrote:

...

Dne 15.1.2015 v 17:53 Ancor Gonzalez Sosa napsal(a):

...
This change introduced in our test:unit task has caused the tests in yast-yast2 to break. https://github.com/yast/yast-rake/commit/b00ca0e8c8208b05698675b0cb4752552de...

That commit runs all tests in a single process, the advantages are:

- faster run (almost by factor 10!)

- proper code coverage (before the change every run rewrote the previous code coverage statistics, at the end the report contained only the coverage from the last run)

See https://github.com/yast/yast-rake/issues/11 for details.

Yep, that was reason for change.

...

...
It's actually not a fault of the change in the task. The problem is that the tests are fragile because they pollute the global namespace in several ways.

Ouch, I didn't realize this side effect...

Well, usually it do not break as you share one helper, so only constants can be redefined, but it is just warning. Problem in yast2 is that it have multiple locations which contain tests, so there can be bigger collision as even helpers are not shared.

...

[...]

...
The weird thing is that I needed to introduce the line marked with a FIXME, which means that closing the default SCR is not as secure as all our tests assume. Looking at the yast2-core source code I'd say that the line is actually needed and we need to update quite some tests and the documentation. But it's the core, so of course I could be wrong.

I have checked the code, and yes, after closing the current SCR you need to set the current one as no default SCR is set:

https://github.com/yast/yast-core/blob/master/wfm/src/Y2WFMComponent.cc#L301

But I'm not much familiar with that part of Yast...

you are right. I remember it wrongly as there was some handling in past, but I removed it and it actually do not fix SCR instance number - https://github.com/yast/yast-core/commit/26d7ae4fbc0d68e5fe38d44bddfa9c7bc26... What we can do is: 1) fix usage 2) behave smarter and select any remaining SCR instance when default is closed as usual use case is one system SCR and one additional ( /mnt one in installation or relative in case of tests ). What do you think? Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Ancor Gonzalez Sosa

20 Jan 20 Jan

11:24

On 01/15/2015 05:53 PM, Ancor Gonzalez Sosa wrote:

...

This change introduced in our test:unit task has caused the tests in yast-yast2 to break. https://github.com/yast/yast-rake/commit/b00ca0e8c8208b05698675b0cb4752552de...

It's actually not a fault of the change in the task. The problem is that the tests are fragile because they pollute the global namespace in several ways. To be honest, I'd expect test from some other repositories to also break. Specially those written when we were still newbies with RSpec.

I did some changes in order to fix it but found another problem. Here is the patch, not intended to enhance or refactor the tests in any way, just to fix the minimum needed to make them run again. https://github.com/ancorgs/yast-yast2/commit/a5c09480081be09d894f2df3a8b36a3...

The weird thing is that I needed to introduce the line marked with a FIXME, which means that closing the default SCR is not as secure as all our tests assume. Looking at the yast2-core source code I'd say that the line is actually needed and we need to update quite some tests and the documentation. But it's the core, so of course I could be wrong.

That's the question I was trying to open in the phone call. We need to fix the broken tests in yast-yast2 and, for sure, in many other modules. How to proceed about the branches? 1) For maintenance branches like SLE12 and 13.2 Do we want to fix it? They work with the version of yast-rake present in SLE12 and 13.2, but we cannot run "rake test:unit" for those branches with yast-rake>0.1.8 (in Tumbleweed at the moment). 2) For master We have to fix it (not a question) but I think that fixing, for example, chrooting of SCR in every single module is wrong. We need to extract this functionality to the ruby bindings and then use them in the different modules. So in my opinion it's time to introduce RSpec helpers for SCR in the ruby bindings. 3) For both (or only for master if we decide not to fix maint. branches) Do we want to fix all the modules right now or only yast-yast2 and wait to see errors in the rest? Keep in mind that in order to experiment the errors you need to be using yast-rake>0.1.8 so the errors could persist unnoticed for quite some time unless we all update the dev tools. Cheers. -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Ancor Gonzalez Sosa

11:51

On 01/20/2015 12:24 PM, Ancor Gonzalez Sosa wrote:

...

On 01/15/2015 05:53 PM, Ancor Gonzalez Sosa wrote:

...
This change introduced in our test:unit task has caused the tests in yast-yast2 to break. https://github.com/yast/yast-rake/commit/b00ca0e8c8208b05698675b0cb4752552de...

It's actually not a fault of the change in the task. The problem is that the tests are fragile because they pollute the global namespace in several ways. To be honest, I'd expect test from some other repositories to also break. Specially those written when we were still newbies with RSpec.

I did some changes in order to fix it but found another problem. Here is the patch, not intended to enhance or refactor the tests in any way, just to fix the minimum needed to make them run again. https://github.com/ancorgs/yast-yast2/commit/a5c09480081be09d894f2df3a8b36a3...

The weird thing is that I needed to introduce the line marked with a FIXME, which means that closing the default SCR is not as secure as all our tests assume. Looking at the yast2-core source code I'd say that the line is actually needed and we need to update quite some tests and the documentation. But it's the core, so of course I could be wrong.

That's the question I was trying to open in the phone call. We need to fix the broken tests in yast-yast2 and, for sure, in many other modules. How to proceed about the branches?

1) For maintenance branches like SLE12 and 13.2

Do we want to fix it?

They work with the version of yast-rake present in SLE12 and 13.2, but we cannot run "rake test:unit" for those branches with yast-rake>0.1.8 (in Tumbleweed at the moment).

2) For master

We have to fix it (not a question) but I think that fixing, for example, chrooting of SCR in every single module is wrong. We need to extract this functionality to the ruby bindings and then use them in the different modules. So in my opinion it's time to introduce RSpec helpers for SCR in the ruby bindings.

Like this, that I will commit to the proper repositories/branches once we answer the question above. https://github.com/ancorgs/yast-yast2/blob/29bfc826c0df463e77fbf0512c272167a...

...

3) For both (or only for master if we decide not to fix maint. branches)

Do we want to fix all the modules right now or only yast-yast2 and wait to see errors in the rest? Keep in mind that in order to experiment the errors you need to be using yast-rake>0.1.8 so the errors could persist unnoticed for quite some time unless we all update the dev tools.

Cheers.

-- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Josef Reidinger

12:14

On Tue, 20 Jan 2015 12:51:08 +0100 Ancor Gonzalez Sosa wrote:

...

On 01/20/2015 12:24 PM, Ancor Gonzalez Sosa wrote:

...
On 01/15/2015 05:53 PM, Ancor Gonzalez Sosa wrote:

...
This change introduced in our test:unit task has caused the tests in yast-yast2 to break. https://github.com/yast/yast-rake/commit/b00ca0e8c8208b05698675b0cb4752552de...

It's actually not a fault of the change in the task. The problem is that the tests are fragile because they pollute the global namespace in several ways. To be honest, I'd expect test from some other repositories to also break. Specially those written when we were still newbies with RSpec.

I did some changes in order to fix it but found another problem. Here is the patch, not intended to enhance or refactor the tests in any way, just to fix the minimum needed to make them run again. https://github.com/ancorgs/yast-yast2/commit/a5c09480081be09d894f2df3a8b36a3...

The weird thing is that I needed to introduce the line marked with a FIXME, which means that closing the default SCR is not as secure as all our tests assume. Looking at the yast2-core source code I'd say that the line is actually needed and we need to update quite some tests and the documentation. But it's the core, so of course I could be wrong.

That's the question I was trying to open in the phone call. We need to fix the broken tests in yast-yast2 and, for sure, in many other modules. How to proceed about the branches?

1) For maintenance branches like SLE12 and 13.2

Do we want to fix it?

They work with the version of yast-rake present in SLE12 and 13.2, but we cannot run "rake test:unit" for those branches with yast-rake>0.1.8 (in Tumbleweed at the moment).

I think as long as fix works with both version of yast-rake we should backport it to maintenance branches, so it will work without problems. For me it is bug, not feature of old versions :)

...

...
2) For master

We have to fix it (not a question) but I think that fixing, for example, chrooting of SCR in every single module is wrong. We need to extract this functionality to the ruby bindings and then use them in the different modules. So in my opinion it's time to introduce RSpec helpers for SCR in the ruby bindings.

Like this, that I will commit to the proper repositories/branches once we answer the question above.

https://github.com/ancorgs/yast-yast2/blob/29bfc826c0df463e77fbf0512c272167a...

I agree. It should be in ruby bindings, but as I commented, it is not so easy to do it properly generic. So maybe we start with small one and then extending it. I prefer to have it close for changes and open for extension principle here, as this should be really backward compatible, otherwise every change can cause breakage of all testsuite, which I would like to avoid.

...

...
3) For both (or only for master if we decide not to fix maint. branches)

Do we want to fix all the modules right now or only yast-yast2 and wait to see errors in the rest? Keep in mind that in order to experiment the errors you need to be using yast-rake>0.1.8 so the errors could persist unnoticed for quite some time unless we all update the dev tools.

Cheers.

As long as we can release and deliver yast to users and customers it is not so critical. So I think we can go with evolution way, not revolution which changes it in all modules. Maybe we just need to check it for new code...BTW what version use travis? It should use latest, not? Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Ladislav Slezak

12:34

Dne 20.1.2015 v 13:14 Josef Reidinger napsal(a): [...]

...

As long as we can release and deliver yast to users and customers it is not so critical. So I think we can go with evolution way, not revolution which changes it in all modules. Maybe we just need to check it for new code...BTW what version use travis? It should use latest,

At Travis we install the latest yast-rake gem from rubygems.org, but it would be possible to use a specific version if needed, just like we do for RSpec. -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak@suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Josef Reidinger

12:38

On Tue, 20 Jan 2015 13:34:27 +0100 Ladislav Slezak wrote:

...

Dne 20.1.2015 v 13:14 Josef Reidinger napsal(a): [...]

...
As long as we can release and deliver yast to users and customers it is not so critical. So I think we can go with evolution way, not revolution which changes it in all modules. Maybe we just need to check it for new code...BTW what version use travis? It should use latest,

At Travis we install the latest yast-rake gem from rubygems.org, but it would be possible to use a specific version if needed, just like we do for RSpec.

So in this case it should fail on travis, when travis run it. So first who touch module need to fix it :) Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Ancor Gonzalez Sosa

13:17

On 01/20/2015 01:14 PM, Josef Reidinger wrote:

...

On Tue, 20 Jan 2015 12:51:08 +0100 Ancor Gonzalez Sosa wrote:

...
On 01/20/2015 12:24 PM, Ancor Gonzalez Sosa wrote:

...
On 01/15/2015 05:53 PM, Ancor Gonzalez Sosa wrote:

2) For master

We have to fix it (not a question) but I think that fixing, for example, chrooting of SCR in every single module is wrong. We need to extract this functionality to the ruby bindings and then use them in the different modules. So in my opinion it's time to introduce RSpec helpers for SCR in the ruby bindings.

Like this, that I will commit to the proper repositories/branches once we answer the question above.

https://github.com/ancorgs/yast-yast2/blob/29bfc826c0df463e77fbf0512c272167a...

I agree. It should be in ruby bindings, but as I commented, it is not so easy to do it properly generic. So maybe we start with small one and then extending it. I prefer to have it close for changes and open for extension principle here, as this should be really backward compatible, otherwise every change can cause breakage of all testsuite, which I would like to avoid.

Yep. That's why in this version I only included the #path helper and the chrooting. I think they are a good starting point because they are generic, low-risk and widely useful. For other things, like stubbing of SCR/WFM calls, we need to come up with a proposal able to satisfy all use cases. Cheers. -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

Josef Reidinger

13:32

On Tue, 20 Jan 2015 14:17:46 +0100 Ancor Gonzalez Sosa wrote:

...

On 01/20/2015 01:14 PM, Josef Reidinger wrote:

...
On Tue, 20 Jan 2015 12:51:08 +0100 Ancor Gonzalez Sosa wrote:

...
On 01/20/2015 12:24 PM, Ancor Gonzalez Sosa wrote:

...
On 01/15/2015 05:53 PM, Ancor Gonzalez Sosa wrote:

2) For master

We have to fix it (not a question) but I think that fixing, for example, chrooting of SCR in every single module is wrong. We need to extract this functionality to the ruby bindings and then use them in the different modules. So in my opinion it's time to introduce RSpec helpers for SCR in the ruby bindings.

Like this, that I will commit to the proper repositories/branches once we answer the question above.

https://github.com/ancorgs/yast-yast2/blob/29bfc826c0df463e77fbf0512c272167a...

I agree. It should be in ruby bindings, but as I commented, it is not so easy to do it properly generic. So maybe we start with small one and then extending it. I prefer to have it close for changes and open for extension principle here, as this should be really backward compatible, otherwise every change can cause breakage of all testsuite, which I would like to avoid.

Yep. That's why in this version I only included the #path helper and the chrooting. I think they are a good starting point because they are generic, low-risk and widely useful. For other things, like stubbing of SCR/WFM calls, we need to come up with a proposal able to satisfy all use cases.

Cheers.

OK, so feel free to create pull request to ruby-bindings ;) I will review it. I usually have ideas when I comment code and not when I write it ;) Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

3394

Age (days ago)

3399

Last active (days ago)

List overview

Download

13 comments

4 participants

participants (4)

Ancor Gonzalez Sosa
Arvin Schnell
Josef Reidinger
Ladislav Slezak

[yast-devel] Change in yast-rake and fragile tests

tags

participants (4)