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

[ADD] initial PoC for tests for OpenUpgrade #506

Merged
merged 1 commit into from
Jun 13, 2016
Merged

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Jun 9, 2016

@hbrunn hbrunn added this to the 9.0 milestone Jun 9, 2016
@pedrobaeza
Copy link
Member

The problem I see is that we are only going to test:

  • The correct run of the script.
  • The checks we want to incorporate, based only in what we figure that we have to migrate

But we are not going to know exactly if the migration is totally correct. These tests can create the false sensation of correct migration even if we don't include any assert.

Anyway, having this clear in documentation, it's a good step to avoid some running issues.

@hbrunn
Copy link
Member Author

hbrunn commented Jun 9, 2016

...and we can (and should) also include measuring coverage, and at the end, only merge stuff which doesn't decrease coverage

@StefanRijnhart
Copy link
Member

🌟 🌟 🌟

Try

postgresql: "9.4"

@hbrunn
Copy link
Member Author

hbrunn commented Jun 13, 2016

@hbrunn
Copy link
Member Author

hbrunn commented Jun 13, 2016

looks like OCA/openupgradelib@e748410 is missing from the openupgradelib travis uses. Will have a look what's going on here.

@pedrobaeza
Copy link
Member

It seems there has no been another release in PyPi with latest changes of openupgradelib.

@pedrobaeza
Copy link
Member

I'm going to make another release.

@hbrunn
Copy link
Member Author

hbrunn commented Jun 13, 2016

@pedrobaeza thanks, the new release surely makes sense. But for the tests, I'll rewrite .travis.yml to pull the git version of openupgradelib in order to have the latest fixes in there. Especially at the start of the version, we'll probably add stuff and then having to release basically every commit will become annoying.

@pedrobaeza
Copy link
Member

@hbrunn, OK, you're right about that point.

# on unreleased features in openupgradelib
- pip uninstall openupgradelib
- git clone https://github.com/OCA/openupgradelib ~/openupgradelib
- pip install ~/openupgradelib
Copy link
Member

Choose a reason for hiding this comment

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

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 54edfd62a42059e796ddf0e8742b1549c7a7927c on hbrunn:9.0-tests into * on OCA:9.0*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f6af3b59f407724907db689c6134d4577d9df538 on hbrunn:9.0-tests into * on OCA:9.0*.

@hbrunn
Copy link
Member Author

hbrunn commented Jun 13, 2016

@pedrobaeza @StefanRijnhart now it's the way I want it, please review

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6de5396 on hbrunn:9.0-tests into * on OCA:9.0*.

@StefanRijnhart
Copy link
Member

Awesome! And it's green. 👍 👍 👍

@hbrunn
Copy link
Member Author

hbrunn commented Jun 13, 2016

I see three thumbs up, so I'll merge

@hbrunn
Copy link
Member Author

hbrunn commented Jun 13, 2016

kidding, but thanks for your enthusiasm ;-)

@pedrobaeza
Copy link
Member

Is the coverage information correct? How do you measure this coverage?

@hbrunn
Copy link
Member Author

hbrunn commented Jun 13, 2016

@pedrobaeza it only looks at the migration scripts themselves: https://github.com/OCA/OpenUpgrade/pull/506/files#diff-6f1f859c6ca5c4cae7310924786cf661R7

@legalsylvain
Copy link
Contributor

Hi,

Well first 💯 👍 for this work (*) ! This a good step forward to improve Openupgrade.

A suggestion :

  • What about generating DB dynamicaly, based on an OCB instance created each time ?

Pro :

  • we could install only "upgradable" module. If lunch is not ported, no need to install it. -> less time during upgrade / update process
  • if something changes in OCB / 8.0, maybe the migration will no longer works, and we could detect it (theoritical case maybe ...)
  • no need for Therp BV to host and maintain db https://doc.therp.nl/openupgrade/databases/for/9.0
  • maybe we could quickly patch OCB / 8.0 branch if there a bug in a demo data that makes the upgrade fail

Cons :

  • more time to generate db
  • other ?

And a question : I remember that for previous migration, it was not possible to run an openupgrade on a db with demo data but I don't remember why. Why it is now possible from 8.0 -> 9.0 ?

kind regard.

(*) 103 thumbs : time to merge... ;-)

@pedrobaeza
Copy link
Member

👍 Merging then

@pedrobaeza pedrobaeza merged commit 3a4570b into OCA:9.0 Jun 13, 2016
@hbrunn
Copy link
Member Author

hbrunn commented Jun 14, 2016

@pedrobaeza thanks for merging

@legalsylvain the time thing is crucial: We only have 50 minutes, then travis will kill the job. Now doing nearly nothing it costs 10 minutes, so I fear we'll get timing problems anyways with a growing amount of modules.

Therp hosting the DB images was now the simplest choice, sooner or later I'll ask the OCA to do so - also makes more sense because then more people have access to it. Or just on github, weird I didn't think of that before.

@legalsylvain
Copy link
Contributor

@hbrunn Thanks for the explanation. (and again for this improvement).

@StefanRijnhart StefanRijnhart mentioned this pull request Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants