faces icon indicating copy to clipboard operation
faces copied to clipboard

TCK Challenge: Test assertions should not make implementation-specific assumptions

Open brideck opened this issue 2 years ago • 7 comments

Challenged Tests: ee.jakarta.tck.faces.test.javaee8.commandScript.Spec613IT#testCommandScript ee.jakarta.tck.faces.test.servlet40.el.Spec1337IT#testResourceEL3 ee.jakarta.tck.faces.test.servlet30.ajax.Issue3097IT#testViewExpired1

TCK Version: Jakarta Faces 4.0.x

Tested Implementation: Open Liberty -- containing MyFaces 4.0

Description: The challenged tests all have assertions that are making assumptions based off of the output that Mojarra produces:

Spec613IT - The test is checking that the web response contains the string ">var foo=function(o){". Unfortunately, MyFaces produces the slightly different output of ">var foo = function(o){" so the test fails. These are obviously the same string on a fundamental level.

Spec1337IT - The test is checking that the page contains the phrase "contains more than one colon". Not having run the TCK with Mojarra, I can't say exactly what the context of this phrase is, but given the nature of the test this is probably either part of an error message or exception text. MyFaces throws an exception here that clearly indicates that there is a problem with the resource, but does not contain this specific text:

Caused by: jakarta.el.ELException: Malformed resource reference found when resolving resource3EL::resourceEL3.gif
    at org.apache.myfaces.el.resolver.ResourceResolver.getValue(ResourceResolver.java:99)
    at jakarta.el.CompositeELResolver.getValue(CompositeELResolver.java:62)
    at org.apache.el.parser.AstValue.getValue(AstValue.java:169)
    at org.apache.el.ValueExpressionImpl.getValue(ValueExpressionImpl.java:190)
    at org.apache.myfaces.view.facelets.el.ContextAwareTagValueExpression.getValue(ContextAwareTagValueExpression.java:100)

Issue3097IT - The test is checking the result page for the string "class jakarta.faces.application.ViewExpiredException". With MyFaces, the correct exception (ViewExpiredException) is thrown and surfaced in the result page, but the result does not contain the "class" keyword, so the test fails. Without knowing the full context of Mojarra's result page it's hard to say why the test writer would have found it necessary to include "class" in the assertion text. Checking for "jakarta.faces.application.ViewExpiredException" should be sufficient to verify correct behavior.

These tests need to be rewritten in an implementation-neutral fashion for a future release of the Faces TCK.

brideck avatar Oct 10 '22 18:10 brideck

From my point of view, this is a valid challenge. @arjantijms @BalusC @tandraschko do you agree?

pnicolucci avatar Oct 12 '22 12:10 pnicolucci

At a glance it indeed seems valid.

For the current version we should exclude those tests then, or, if reasonably possible, update the tests in such a way that no new or different functionality is tested and then ask for the common exception (contradictio in terminis, I know) again to allow the update.

@BalusC wdyt?

arjantijms avatar Oct 12 '22 13:10 arjantijms

@BalusC any input? Thanks!

pnicolucci avatar Nov 07 '22 00:11 pnicolucci

Test assertions should not make implementation-specific assumptions

Completely correct.

Spec613IT

Test should be rewritten if window.foo is available at all and is of type function (and thus not e.g. string).

Spec1337IT

Test should be rewritten to check if jakarta.el.ELException is thrown at all.

Issue3097IT

Test should be rewritten to check if the function behind faces.ajax.addOnError() is called at all.

BalusC avatar Nov 07 '22 12:11 BalusC

@BalusC @arjantijms given the discussion here I think we can accept this challenge.

pnicolucci avatar Nov 07 '22 12:11 pnicolucci

I can open a new challenge, if desired, but as we peel the onion on some tests blocked by other challenges I've found 2 more tests that would fit here: ee.jakarta.tck.faces.test.javaee8.cdi.Spec527IT#testInjectFacesContext ee.jakarta.tck.faces.test.javaee8.cdi.Spec1309IT#testInjectExternalContext

Both tests are explicitly testing that injected Contexts are the com.sun.faces implementation, e.g. com.sun.faces.context.ExternalContextImpl

brideck avatar Nov 15 '22 19:11 brideck

@arjantijms are you ok adding the tests above from @brideck or would you prefer a new challenge?

pnicolucci avatar Nov 15 '22 20:11 pnicolucci

Addressed by https://github.com/jakartaee/faces/pull/1728 which updated the tests and should complete the challenge.

arjantijms avatar Nov 23 '22 20:11 arjantijms