Fix upgrade modifications

[andresmgot]

Description of the change

Fixes #1479

Regression added in #1472

We were not taking into account the auto-resolved repository in order to populate the property repoName.

Since a Container is difficult to test with unit-tests I have covered this fix with the end-to-end test for upgrading.

[absoludity]

Since a Container is difficult to test with unit-tests I have covered this fix with the end-to-end test for upgrading.

I don't see an issue here since what you're testing doesn't involve any network, but we need to be careful using e2e tests for things which should be unit tested (end up with more complicated e2e tests, slower e2e tests, etc., if the pattern is followed).

Not sure why a Container is difficult to unit-test? In particular, here we should be able to do something very similar to https://github.com/kubeapps/kubeapps/blob/master/dashboard/src/containers/HeaderContainer/HeaderContainer.test.tsx#L27 (ie. create a state store with the initial state, verify that the props passed to the component include the correct repoName depending on that state).

[andresmgot]

I don't see an issue here since what you're testing doesn't involve any network, but we need to be careful using e2e tests for things which should be unit tested (end up with more complicated e2e tests, slower e2e tests, etc., if the pattern is followed).

I agree. In this case I added that to the e2e test because it involved a user-facing functionality. I wanted to test that the values changes are preserved when upgrading which is very valuable. It's also not adding more time to the test (in this case).

Not sure why a Container is difficult to unit-test? In particular, here we should be able to do something very similar to https://github.com/kubeapps/kubeapps/blob/master/dashboard/src/containers/HeaderContainer/HeaderContainer.test.tsx#L27 (ie. create a state store with the initial state, verify that the props passed to the component include the correct repoName depending on that state).

Well, yes, it can be tested but we are setting up an artificial environment and we are setting the expected values to get the expected result (which is only to forward a boolean). In this case I think it's more useful to have an e2e test but I am fine adding the unit test as well.

[absoludity]

Well, yes, it can be tested but we are setting up an artificial environment and we are setting the expected values to get the expected result (which is only to forward a boolean).

Yep, a unit test ensuring that the container converts the redux state into the correct args... it is testing exactly the mapStateToProps. It can seem a bit pointless when it's such a small change, but it also sets up the next person to be able to add to the test, rather than adding more to the e2e test.

In this case I think it's more useful to have an e2e test but I am fine adding the unit test as well.

Thanks :)