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

Use travis-ci for testing #636

Closed
wants to merge 10 commits into from
Closed

Use travis-ci for testing #636

wants to merge 10 commits into from

Conversation

juzna
Copy link
Contributor

@juzna juzna commented Apr 27, 2012

When you enable a Service hook in github, every commit will be automatically tested in the cloud and you'll see the results like http://travis-ci.org/#!/juzna/nette

@juzna
Copy link
Contributor Author

juzna commented Apr 27, 2012

sorry there is some mess atm, I'm working on this pull and have to push to get it to travis... will clean it afterwards

@juzna
Copy link
Contributor Author

juzna commented Apr 27, 2012

All the tests are passing, except one on PHP 5.3 which seems to be bug in PHP and three on PHP 5.4, which are new bugs introduced there.

@juzna
Copy link
Contributor Author

juzna commented Apr 27, 2012

Just a note, if you wanna add a repo from GitHub Ogranization, it's not that simple (two clicks) as from normal user. Follow this manual, you actually just need to fill in two fields (manually).

@juzna
Copy link
Contributor Author

juzna commented May 15, 2012

Another recommendation: if we use travis, we would see if each pull request breaks some tests (see example)

@fprochazka
Copy link
Contributor

+1 :)

@dg
Copy link
Member

dg commented Aug 22, 2012

I do not understand why I have to add file .travis.yml to the framework. I do not like it :-/

@juzna
Copy link
Contributor Author

juzna commented Aug 23, 2012

It defines how the tests should be executed, so that Travis can do it automatically. And so that it can do it for all the forks as well.

The principle is similar to composer - single file in repo's root.

But you're right that we need to be careful not to end up like symfony with so much crap in root dir.

@juzna
Copy link
Contributor Author

juzna commented Sep 1, 2012

See forum for more details, motivation and discussion about pros and cons.

Build Status

Edit: it was passing before, but after we added also memcache tests, it doesn't work anymore :(
Edit 2: passing again :)

@fprochazka
Copy link
Contributor

❤️ 👏 👍

@JanTvrdik
Copy link
Contributor

👍 😍 🙏

@juzna
Copy link
Contributor Author

juzna commented Sep 1, 2012

Special thanks to @hosiplan for all of these

@hrach
Copy link
Contributor

hrach commented Sep 1, 2012

Sorry but skipping the test just because php 5.4 has a bug is not propper solution.
a) you will not realise that the bug is fixed
b) you have false certainty that framework works fine with 5.4.

@juzna
Copy link
Contributor Author

juzna commented Sep 1, 2012

@hrach that is dicussed on forum

@fprochazka
Copy link
Contributor

@hrach OK then, send patch to PHP, we all will be gratefull.

@hrach
Copy link
Contributor

hrach commented Sep 1, 2012

@hosiplan well, that's typicall (arogant?) answer if somebody have sth against your work, the "do it yourself". No, I do not have knowledge to do that, but it does not change the fact this approach is not right. Obsession with green dot is nice, but there are some borders, which should not be passed.

Btw, @juzna is quite skilled in hacking php. He should be able to do that, shouldn't he? From that bug, I believe, it's just about passing some parameter...

@juzna
Copy link
Contributor Author

juzna commented Sep 1, 2012

@hrach we need all green in order to use Travis properly (reasoning on forum). Sure this is not the most correct solution ever, but it's best we can get. Little cons (some small test skipped) and big pros (having travis).

Always, balance needs to be found.

@fprochazka
Copy link
Contributor

@hrach the problematic test is marked as skipped - which doesn't mean resolved, it mean it's skipped.

@hrach
Copy link
Contributor

hrach commented Sep 3, 2012

Is to possible run test twice? Would be nice if we can test Nette\Database against multiple databases (postgre).

@dg dg closed this in 4662337 Sep 3, 2012
@fprochazka
Copy link
Contributor

@dg I don't think this is closed. You haven't merged configuration of memcache & test fixes. There should be as little as possible of skipped tests.

If the tests are failing, there is no point of having pullrequests tested.

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

@dg unluckily .travis.yml has to be in project root

@dg
Copy link
Member

dg commented Sep 3, 2012

juzna Tests: separated Form tests with invalid characters … 203388c --- will not be merged
juzna Tests: skipped tests which fail due to bug in PHP … acf5d76 --- will not be merged
juzna Tests: travis-ci for Continous Integration 7112e4f --- merged
juzna Tests: make sure session path is valid 734f0c4 --- why is it needed?
juzna Tests: fixed order of method parameters … d766dd5 --- must be solved on different level
juzna Tests: scream when php.ini not found and pass full path to test workers a4af60a --- merged
juzna Tests: use default php.ini if none specified 2ea3ecc --- will not be merged
juzna Tests: removed unnecessary options from travis 013c255 --- merged
juzna Tests: memcache on travis d451d0a --- merged
juzna Tests: separate php.ini for win/unix  7dc6538 --- will not be merged

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

Reasoning why are failing tests skipped was written on forum:

Limitation:
We need to have all existing tests passing to gain all benefits. Or at least they have to be skipped, so that final result is passing. Only then, when something gets broken, we'll get failed result which will indicate a problem.

It covers:
Tests: skipped tests which fail due to bug in PHP … acf5d76
Tests: fixed order of method parameters … d766dd5

Sure this is just a workaround and should not exist at all if PHP worked properly. But again, ...

There are pros and cons for this solution, but I believe it's worth having it included. If that happens, we'll be able to spot problems early and save our time.

Tests: make sure session path is valid 734f0c4
Test was failing in wrong place when session dir was configured incorrectly

Tests: use default php.ini if none specified 2ea3ecc
Tests: separate php.ini for win/unix 7dc653
If php.ini is not specified, the system one is used which means tests are executed in unpredictable environment. For example date.timezone may be set to something else than expected by the tests, or it may not be configured at all and most tests will fail then.
Also, one must think about this everytime he runs tests.

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

Regarding php's configuration - there should be php.ini pre-defined also for linux (now there is one only for windows), so that linux users can pass that one into -c option. This one can then be uded by travis as well.

@dg
Copy link
Member

dg commented Sep 3, 2012

@juzna „We need to have all existing tests passing to gain all benefits.“

Yes, we need to have all existing tests passing - its the purpose of testing. But we must never modify tests in order they will pass.

ad 203388c: compare this totally stupid solution with solution 0e79764

ad d766dd5: this test discovered very strange and important bug, so you changed the test? WTF??

ad 734f0c4: how is possible to have session dir incorrectly configured? There is ini_set('session.save_path', TEMP_DIR);

ad 2ea3ecc: unpredictable environment I have solved here 2ea3ecc. Correct php.ini should be always specified, with no magic in TestRunner.

@dg
Copy link
Member

dg commented Sep 3, 2012

Yes, we need php.ini for linux, but this one is not working 7dc6538#L3L-1. Did you test it?

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

@dg I agree that some commits were not best solution, but only best I had. I very appreciate your critics and I see you're right. Thanks

About php.ini - that one works for me (Ubuntu 10.04) and also worked on Travis. What problem does it causes to you?

@fprochazka
Copy link
Contributor

AFAIK it worked for me and @JanTvrdik too.

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

Btw about that Forms.invalidInput.002.php, I thought it tested also different kind of invalid input (in addition to invalid encoding) because of this unexpected data. That's the reason I kept it and moved invalid encoding into another test. Result: we could test separately validation in forms and in encoding.

Though I agree it's not best, again only the best I had.

@dg
Copy link
Member

dg commented Sep 3, 2012

ad php.ini: There are a lot of tests (i.e tests\Nette\Utils\Strings.compare().phpt) which require mbstring, so it is surprising it works without mbstring in php.ini file.

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

I haven't tested on win since I'm a proud non-owner of that OS ;) On linux it worked because we have mbstring bundled in php executable, which is quiet common I guess.

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

btw 0e79764 doesn't solve it properly. On php 5.4 it's still broken:

-> Nette\Http\Request invalid encoding.
   file: /home/juzna/projects/nette/tests/Nette/Http/Request.invalidEncoding.phpt

Failed asserting that "v" is identical to expected "vž" in file /home/juzna/projects/nette/tests/Nette/Http/Request.invalidEncoding.phpt on line 103

and it seems to break also two tests on 5.3:

-> Nette\Utils\Strings::fixEncoding()
   file: /home/juzna/projects/nette/tests/Nette/Utils/Strings.fixEncoding().phpt

Failed asserting that "ab" is identical to expected "ab" in file /home/juzna/projects/nette/tests/Nette/Utils/Strings.fixEncoding().phpt on line 37

-> Nette\Utils\Strings::checkEncoding()
   file: /home/juzna/projects/nette/tests/Nette/Utils/Strings.checkEncoding().phpt

Failed asserting that TRUE is FALSE in file /home/juzna/projects/nette/tests/Nette/Utils/Strings.checkEncoding().phpt on line 22

(I hope I'm right here and there is not another stupid mistake somewhere else)

@dg
Copy link
Member

dg commented Sep 3, 2012

So all of there extensions (https://github.com/nette/nette/blob/835d742ffa3874c558153e9c09f381ccebd74c89/tests/php.ini-win) are default? Sorry, I did not know that.

@dg
Copy link
Member

dg commented Sep 3, 2012

What is your Multibyte string engine (in phpinfo)?

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

On PHP 5.4:

Multibyte Support    enabled
Multibyte string engine libmbfl
HTTP input encoding translation disabled
libmbfl version 1.3.2

On PHP 5.3 the same, but it doesn't provide info about version.

@dg
Copy link
Member

dg commented Sep 3, 2012

I found it: changed error handling of invalid UTF-8 based on the unicode consorsium's recommendation: moriyoshi/libmbfl@66c371c - it skips valid \xC5\xBE after invalid \xC4.

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

I've got the same failures as in last travis build

@juzna
Copy link
Contributor Author

juzna commented Sep 3, 2012

About the extensions on Linux - sometimes they're bundled, sometimes not. It depends on the person who compiles PHP.

At least on Ubuntu it seems they're bundled when installed from apt-get repository, though I haven't found any information mentioning this. On my machine I compiled php myself.

@milo
Copy link
Member

milo commented Sep 4, 2012

K php.ini na Linuxu... Třeba na Debianu je php v balíčku kompilováno s --with-config-file-scan-dir=/etc/php5/$SAPI/conf.d. To může budit zdání, že některé moduly jsou již zakompilovány natvrdo, ale není tomu tak, protože když se předá parametr -c moje-php.ini ostatní ini z adresáře /etc/php5/$SAPI/conf.d se načtou taktéž.

Eh, sorry, nevšiml jsem si angličtiny :-)

@juzna
Copy link
Contributor Author

juzna commented Sep 4, 2012

@dg it all looks great, thanks!

Perhaps one more small fix to prevent such strange errors?

Would you also like to set up GitHub hook to travis, so that CI builds get triggered automatically?

@dg
Copy link
Member

dg commented Sep 4, 2012

Hook would be great.

@juzna
Copy link
Contributor Author

juzna commented Sep 4, 2012

@dg
1/ go to travis, sign in
2/ go to profile, enable nette
3/ set hook on github
4/ click Test hook to trigger first build manually
5/ watch the build in real time with fingers crossed :)

@dg
Copy link
Member

dg commented Sep 4, 2012

Fuj, zelená! http://travis-ci.org/#!/nette/nette

@juzna
Copy link
Contributor Author

juzna commented Sep 4, 2012

👏 👍

@dg
Copy link
Member

dg commented Sep 4, 2012

Good job, thanks. I hope this is the end of surprising questions: "And on your computer all tests are passing?" ;-)

@fprochazka
Copy link
Contributor

👏 👍 I won't ask again, I promise :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants