Support for Continuous Integration

[lhchavez]

It is super useful to have an automated way of running the whole suite of tests automatically on every pull request and commit, which reduces a bit of verification burden off maintainers and ensures that no change ever introduces regressions. This can be trivially achieved by using a service such as Travis CI, which is free for open source projects.

[balthisar]

Not sure why this was closed. The Bash scripts did not include this topic. Re-opening. Sorry for any confusion delivered via email.

[lhchavez]

That was probably my fault. I included the string "fix ${URL}" in the commit message, and GitHub automatically closed the issue based on that.

[balthisar]

This will be a longer, more in depth discussion. Here are some back references: #330, #269, #261, and #175.

So far, there's not been any real interest. Complicating things, in some cases we expect (or get!) different results on different platforms, meaning Travis good 99% of the time, but not 100% of the time.

I'm open to it, but there's generally never been much of a case for it.

[lhchavez]

Ah I did search the issues for "Travis", "CI", "Continuous Integration", but failed to also look at the closed ones.

I completely agree that developers should test their changes before sending them over for review and have been heavily pushing pre-upload tests on all the projects I've worked on. They are amazing and do reduce the amount of noise that even gets to the PR stage. Unfortunately, git does not support automatically enforcing these :( So CI is the next-best thing I know of.

Then again, as you mention, different results on different platforms is a real issue. Not all developers have access to all combinations of OSs, and I believe CI can help here. Throwing a Windows-specific CI like Appveyor into the mix, and you should be able to get 99.5+% since you'll be able to cover the three most popular OSs.

In any case, it seems like my case is not any stronger than the previous ones that have already been rejected, so I'm happy that at least the improvement to the scripts was accepted.

[balthisar]

I'll leave it open for discussion. Like I said, I'm not against it. It would save me from spinning up a Linux VM for testing, for example. Or a Windows VM for that matter.

The Holy Grail would be something that can test Raspberry Pi, Windows, some variation of Linux, and macOS. As you can see, we often delay commits because we like to have confirmation from different environments. Sometimes, though, it's obvious that a change is not platform specific, and so those are usually merged without too much formality.

Even with this, we still miss things on older platforms. I know @geoffmcl tests on Raspberry Pi and Windows, but I don't know which version of Windows. I only test current macOS and latest LTS Ubuntu, and up-to-date Windows 10 (but only if I expect platform differences). Windows also has API differences, e.g., we don't use positional printf arguments because there's no early Windows support. Now this is reminding me to check if there are any differences between Windows MSVC and Windows using the neat Ubuntu subsystem.

Maybe there's a case for Travis/Linux just for PR's, and then whoever merges the code still checks the different environments.

Like I said, I'm open to the conversation.

[balthisar]

I'll add, it's always the developers' responsibility to check things before posting a PR, but in the end a CI solution could save the maintainers some time by having Github automatically reject things that don't pass the minimum level of regression testing.

[lhchavez]

Yes, this is not supposed to be an excuse for developers to be lazy, only a safety net so that they can see exactly what is expected to be tested in all platforms. That way you, the maintainers just need to do manual testing once the minimum bar has been met. I know this would have helped save both me and @geoffmcl some time in my previous PRs because even though I did perform due diligence in running the tests, the way I ran the scripts was wrong and did not catch some regressions I introduced :( The documentation has since been updated, so nobody should use this as an excuse for the future.

Regarding platform coverage, Travis can get you one of {Precise,Trusty} and one of these versions for macOS, Appveyor can get you one of VS {2013,2015,2017} (maybe more, I'm not too familiar with that service). As for the RPi, I can probably help get the needed environment so that GitHub can talk to a device (that somebody else might need to provide and maintain) on every PR.

[geoffmcl]

@lhchavez thank you for this repeated idea... as @balthisar pointed out there have been numerous back reference to this topic - Travis CI...

Sometimes this seems to become divisive - but it should not be so...

As I have said in numerous previous comments, I, personally, do not see any benefit here... Yes, it is free, it is there, but really what does it really add?

I am 100% sure some projects benefit from this... so why not Tidy?

It seems to me it took a long time to (a) get these regression tests into order, and (b) separate them from the main source repo. Thanks to @balthisar for a lot of that, although I did not agree with some name changes at the time, it has matured as a separate test site... and it works well...

Now the whole idea of adding a git submodule puts them back together... Why?

Who benefits from this?

Yes, there are two levels to consider - simply maintainers and contributors - who gains?

From the contributors point of view, it seems an easy step to pull the next tidy-tests repo, and run all the regression tests, and check out your work. Yes, you pointed out maybe our documentation of this step was lacking, but I immediately added your suggestion for this. And maybe the test scripts can still be improved... This test can be a simple script... run seconds after your build...

From the maintainers point of view, I have multiple t-xxx.bat files, so I can quickly check out a new branch, or a new forked branch, an idea, and run the regression tests... So what do I gain from Travis CI? Except more work in maintaining the submodule...

Now I am lucky! My own personal compile farm includes Ubuntu 14.04 64-bit, Ubuntu 16.04 32-bits. Windows XP 32-bit MSVC8 (2005), Widnows Vista 32-bit MSVC10 (2010). Windows 10, 32 & 64 bit, MSVC14 (2015), and from the earlier back references, added RPI2, ARM7, 32-bit... and thinking about getting a RPI3, 64-bits... so do I need Travis CI, even if it can duplicate some of those environment? I think not!

I will repeat. I am not against this! Just do not see the need, or benefits... yet...

But as @balthisar said somewhere, always open to discussion... maybe I am wrong! Stupid, and just do not see the point...

Really, thanks for your great additions to tidy code... this is most appreciated... and hope for more...

Let's not get stuck, or unstuck, on Travis CI... please... thanks...

[lhchavez]

All right, very last attempt, I promise :)

The one thing I personally found difficult about having a separate test repo is the fact that they can not be in sync. The git submodule approach I followed was just the path of least friction to get this working, but I think that's suboptimal in the long run (as I outlined in #546 ), and the only way to ensure that the code and tests are updated in tandem is for them to be in the same repository (but I didn't want to pitch that idea at first, one change at a time). If this is something that the tidy project does not want to do in the long run (for any reason), then I agree that the maintenance burden is going to be higher and the false positives / negatives will greatly reduce the value of any CI (or even make it negative!).

Leaving CI aside, I think there's still some merit to having the tests and code being in the same repository: a make/cmake target can be added to run the whole suite of tests (so make && make test or ctest could be a shortcut for contributors). You also get a consistent set of tests for the code at any given point in time, no more branch juggling.

[geoffmcl]

@lhchavez thanks for the further feedback... that is no problem... I left it a few days in case others wanted to chime in...

I think the important thing to understand is that these are not Unit Testing, nor perhaps Integration, System nor Acceptance testing, which one would expect to be part of the main source, and kept 100% in sync with such source...

They are what we have called Regression testing, and in general, do not have to be 100% in sync with the main source...

While over the last weeks, month or so, there have been a number of changes, which required updating the tests at the same time... this should not happen that often... As you may have seen the version 5.5.7 in tests stayed the same up until tidy 5.5.15...

And these changes do require a lot of branch juggling for development testing, where both need to be changed at the same time...

But in general, I really do not see much of a burden in the following normal procedure, which could be scripted -

$ cd ../../../tidy-html5-tests/tools-sh
$ git pull
$ ./run-tests.sh

Also understand, they were originally in the same source, and we deliberately chose to move them out... like we also moved out the Doxygen API document building... tidy site documentation, etc, etc... so this was not by accident...

I think I have said everything I need to say about CI ;=))

I hope this, and the PR #546 can be closed, and we move on... but as always thanks for the ideas, support and feedback... this is very much appreciated...