Skip to content

Conversation

@danxuliu
Copy link
Member

This pull request adds a workaround for recent JavaScript unit test failures in Drone.

When using fake servers with Sinon.JS, the JavaScript test framework, the XHR objects are also fake. Both the fake XHR and server come from nise, which is a dependency and subproject from Sinon. In nise 1.3.3, which is used by Sinon.JS since 5.0.8, setRequestHeader of XMLHttpRequest was modified to normalize the header values (as requested by the spec), but since then only string values are accepted; null or integer values can no longer be passed to setRequestHeader, as it expects the replace function to be available in the object. However, in the tests null and integer values are passed to setRequestHeader, which causes them to fail.

Null values are passed due to oc_requesttoken being null. oc_requesttoken is set to the data-requesttoken attribute of the head element when js.js is loaded. However, in the unit tests, that attribute is not set anywhere, so oc_requesttoken ends being null (and, in turn, OC.requestToken too, as it is set based on os_requesttoken also when js.js is loaded).

oc_requesttoken is passed as the value of the requesttoken header for every AJAX call that is not cross domain. In the unit tests some AJAX calls are seen as cross domain and others are not (if not explicitly given in the options of the call, whether an AJAX call is cross domain or not is automatically set by jQuery based on the URL; it seems that some of the URLs used in the tests are wrong, but that is a different issue); those tests that use an AJAX call that is not cross domain fail, as normalizeHeaderValue in the fake XMLHttpRequest tries to call replace on a null object.

Fixing that by initializing oc_requesttoken in specHelper.js to an empty string before every test is run is not enough, though, as there are other places in the code in which non-string values are passed as the header value to setRequestHeader.

In the propFind function of davclient.js, which is an external dependency, the value of the Depth header is passed as an integer to setRequestHeader. The depth is a parameter of the propFind function, so it could be passed as an integer to that function... but then it would not properly handle a depth of 0, as it makes an strict comparison against an integer.

Both Firefox and Chromium accept passing non-string values to their setRequestHeader implementation, and there is currently a pull request in nise to allow again non-string values to be passed to setRequestHeader. It is not clear yet whether nise got too restrictive or the code calling setRequestHeader was too loose. Given that davclient.js is an external dependency, as a temporary measure Sinon version is forced to be 5.0.7 at most until either Sinon or davclient.js are updated.

@nextcloud/javascript

When using fake servers with Sinon.JS, the JavaScript test framework,
the XHR objects are also fake. In Sinon 5.0.8 the "setRequestHeader" of
XMLHttpRequest was modified to normalize the header values (as requested
by the spec), but since then only string values are accepted; null or
integer values can no longer be passed to "setRequestHeader", as it
expects the "replace" function to be available in the object. However,
in the tests null and integer values are passed to "setRequestHeader",
which causes them to fail.

Both Firefox and Chromium accept passing non-string values to their
"setRequestHeader" implementation, and it is done, for example, in
davclient.js; it is not clear yet whether Sinon got too restrictive or
the code calling "setRequestHeader" was too loose. Given that
davclient.js is an external dependency, as a temporary measure Sinon
version is forced to be 5.0.7 at most until either Sinon or davclient.js
are updated.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu added the 3. to review Waiting for reviews label May 28, 2018
@danxuliu danxuliu added this to the Nextcloud 14 milestone May 28, 2018
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Nice explanation.
Let's wait for tests 🚀

@MorrisJobke
Copy link
Member

JSUnit tests work - the failure is due to a docker timeout in the mysql container 🙈

@MorrisJobke MorrisJobke merged commit 4ea7dbb into master May 28, 2018
@MorrisJobke MorrisJobke deleted the limit-sinon-version-to-5.0.7-at-most branch May 28, 2018 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants