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

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

Closed
brideck opened this issue Oct 10, 2022 · 8 comments · Fixed by #1728
Closed

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

brideck opened this issue Oct 10, 2022 · 8 comments · Fixed by #1728
Labels
accepted Accepted certification request challenge TCK challenge

Comments

@brideck
Copy link
Contributor

brideck commented Oct 10, 2022

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.

@pnicolucci
Copy link
Contributor

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

@pnicolucci pnicolucci changed the title TCk Challenge: Test assertions should not make implementation-specific assumptions TCK Challenge: Test assertions should not make implementation-specific assumptions Oct 12, 2022
@arjantijms
Copy link
Contributor

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?

@pnicolucci
Copy link
Contributor

@BalusC any input? Thanks!

@BalusC
Copy link
Member

BalusC commented Nov 7, 2022

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.

@pnicolucci
Copy link
Contributor

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

@arjantijms arjantijms added the accepted Accepted certification request label Nov 8, 2022
@brideck
Copy link
Contributor Author

brideck commented Nov 15, 2022

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

@pnicolucci
Copy link
Contributor

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

BalusC added a commit that referenced this issue Nov 15, 2022
Remove assertion of rendered markup; asserting behavior was sufficient
BalusC added a commit that referenced this issue Nov 15, 2022
Test should assert if ELException is thrown, not inspect its message
BalusC added a commit that referenced this issue Nov 15, 2022
Fix failing assertion of ViewExpiredException class
@arjantijms
Copy link
Contributor

Addressed by #1728 which updated the tests and should complete the challenge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted certification request challenge TCK challenge
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants