[vlc-devel] [vlc-commits] tests/tls: Disable checks for ALPN on apple platforms

[David Fuhrmann]

> Am 01.11.2016 um 21:47 schrieb Rémi Denis-Courmont <remi at remlab.net>:
> 
> Le tiistaina 1. marraskuuta 2016, 18.22.41 EET David Fuhrmann a écrit :
>> I disabled a test for a distinct feature (...)
> 
> You disabled a test for necessary/relied-upon and documented behaviour of the 
> API. This is wrong and needs reverting. Period.

I do not think 100% ALPN support is „necessary" for VLC. It might be needed for full support of HTTP/2, but is definitely not mandatory for HTTP/1.1.

You documented (cc80d6a2) and added (19e7f0ed) the behavior after the securetrcansport plugin was added (673d45d0). So one could argue that the documentation is wrong and does not reflect the current state of all TLS plugin functionalities.

I already made my point why tests for a certain feature should be only enabled after that feature was implemented.
So I do not agree with any revert of any patch of me, in that regard. But I’m happy to help finding better solutions, if we can constructively discuss about pros and cons.

> 
>> You are arguing that you want to use test failures to track missing
>> functionality. I completely disagree with that. 
> 
> No, I did not argue that missing features should be tracked by test cases 
> (although I note thast TDD pretty much consists of exactly doing that). I 
> argued that removing test because true positive failures was wrong.

Tests fail because of a missing feature, not because of a bug. I stand to that opinion, even if you might interpret it differently.

> 
> Meanwhile, you argued that:
> "(...) tests should not be misused for new (...) functionality. There is trac 
> for that. but this is what you do here“

I should have added „in my opinion“ here, see above.

> 2) I completely disagree with the whole premise, and so do well-known industry 
> best practices. Specifically, I think that test cases should be added as early 
> as possible. And as a corrolary, new features would be the very best 
> candidates for test cases.

I am not against adding test cases as early as possible. I’m just against a setup where a test for missing features completely block CI, nightly builds and does not give visibility for other failing tests.

> 3) TLS ALPN is not a "new" feature anyway, and it was not a new feature when 
> the test cases for it were added either. As a matter of fat, the VLC API was 
> added in August 2014, while the test cases were added in January 2016.

I think its not very relevant how new the feature is.

> 4) TLS ALPN is also not a "missing" feature, since all the reference TLS plug-
> in support it. Whether you like it or not, the GnuTLS plug-in is the only 
> reference implementation of the VLC TLS API. Historically, Gildas insisted on 
> making it a plug-in only to avoid libvlc proper depending on GnuTLS, not 
> really to support alternate implementations (since OpenSSL was hopeless). You 
> cannot expect VLC developers at large to support an alternate implementation 
> that targets a proprietary OS and a closed-source (TLS) back-end, requires 
> expensive and dedicated hardware (oh, at least it is not using a different 
> programming language!). Likewise VLC developers should be expected to take 
> care of the Qt GUI, not the MacOS one.

Please stop the trolling, it does not belong here.

> 
> Not to mention that the main, if not only, rationale for the SecureTransport 
> plug-in seems to be working around the GPL, and by extension, my and other 
> people´s copyright license.

Neither gnutls nor the VLC gnutls plugin is GPL. And I’m pretty sure that this was definitely not the main reason. Also, its out of topic for this thread.

> In other words, MacOS/iOS stuff is the responsibility, both morally and 
> practically, of the MacOS/iOS developers *only*.

Yes, thats why I’m trying to maintain a usable test suite for the macOS platform with that patch.

> Well, I do. That test checks for behaviour that the HTTP plug-in depends on. 
> If that test is disabled, nothing protects the relevant parts of the HTTP 
> plug-in from bugs and regressions.

If I understood your recent patch (677948a635307904117a6f104df0ebf97b56e724) correctly, it looks securetransport now correctly fails if ALPN is requested. I assume your HTTP code shall cleanly handle failing TLS plugins, so I do not see any problem at a first glance.

Best regards,
David