You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@johnzon.apache.org by "jungm (via GitHub)" <gi...@apache.org> on 2023/04/21 13:15:25 UTC
[GitHub] [johnzon] jungm opened a new pull request, #101: TCKs in github actions
jungm opened a new pull request, #101:
URL: https://github.com/apache/johnzon/pull/101
- Disables running TCKs by default, introduces `tck-jsonb` and `tck-jsonp` profiles to do so
- Seperately runs JSON-P and JSON-P TCKs in GitHub Actions as part of CI (without failing the CI run if TCKs are failing. Might change this in the future when johnzon is passing all TCKs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #101: TCKs in github actions
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1518711715
To be honest tck are just part of the test suite and when i did personally setup them first in projects it was in the impl module, not outside so a split is something weirdish to me.
CI wise we want to enforce the coverage we have so if we pass 80% of tck we want it ran for any pr probably.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #101: TCKs in github actions
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1517827017
Why is it skipped by default? I would enable them and if you need to drop them just exclude the module with maven (`-pl !$module`), wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] jeanouii commented on pull request #101: TCKs in github actions
Posted by "jeanouii (via GitHub)" <gi...@apache.org>.
jeanouii commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1540167187
Do we want to rebase and merge this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #101: TCKs in github actions
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1541445374
From the previous comment I'm tempted to say rebase => probably, merge => no with the main blocker having a toggle for tck instead of running what passes by default.
Other open point: why running them in a dedicated module and not in core and jsonb modules as it should be by construction (splitting the module is okish for complex builds like tomee but for trivial ones like batchee or johnzon, not sure)
wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] jeanouii commented on pull request #101: TCKs in github actions
Posted by "jeanouii (via GitHub)" <gi...@apache.org>.
jeanouii commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1520608104
I'm against skipping the TCK. They are part of the test suite and I want to know if a commit introduces a backward compatibility issue or a TCK issue.
> skipped them by default to not break the build just because of a tck failure
This is exactly why tests are useful.
If the build is red, we have 2 options:
- fix our code
- fix or exclude the related test
Either way, it can't be a silent decision/action.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] jungm commented on pull request #101: TCKs in github actions
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1517832333
skipped them by default to not break the build just because of a tck failure, imho that behavior can be changed when tcks are passing
Or do you think we should just break the build by default? Can understand that this is possibly unwanted complexity in poms
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] jeanouii commented on pull request #101: TCKs in github actions
Posted by "jeanouii (via GitHub)" <gi...@apache.org>.
jeanouii commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1520633051
Ohhh I see.
Yes this is work in progress but I understand your point now.
I rebased the other PR and ran it. I'll copy the results so we can collaborate and get it merged asap on master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] jungm commented on pull request #101: TCKs in github actions
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1518257255
imho excluding failing tests isn't really a fix, though I can agree on using sth like `-pl !tck` instead of just skipping TCKs altogether by default
CI wise wdyt in general of splitting up in "johnzon itself builds" and "johnzon is TCK compliant"? I can imagine that when TCKs are done that's not really needed anymore and failing TCKs are a reason to fail the CI run
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TCKs in github actions [johnzon]
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm closed pull request #101: TCKs in github actions
URL: https://github.com/apache/johnzon/pull/101
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] jungm commented on pull request #101: TCKs in github actions
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1520624144
tbh the whole reason I made this PR was so we can get the TCK test suite merged without breaking builds on the `master` branch. Splitting out TCKs as a seperate job in CI and disabling them by default _for now_ seemed like a good idea to achieve that for me
Feel like this PR can be closed if either:
- the `jakartaee-10-tck` branch is kept as is to fix falling TCKs
- TCK fixes should be PRed against `jakartaee-10-tck`
- `jakartaee-10-tck` gets merged into `master`, either as is or disabling currently failing TCKs and PRs that fix those reenable them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TCKs in github actions [johnzon]
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1759435744
Closing this, both JSON-P and JSON-B TCKs are passing now so no need to add extra complexity in CI anymore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] rmannibucau commented on pull request #101: TCKs in github actions
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1517902332
@jungm the broken build can be "fixed" by excluding the broken tests for now, then we remove the exclusions once they pass but dropping some coverage by default sounds worse than the opposite without more thoughts to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] jeanouii commented on pull request #101: TCKs in github actions
Posted by "jeanouii (via GitHub)" <gi...@apache.org>.
jeanouii commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1544595216
I don't mind. I split them apart because I thought it was more clear and simpler to activate/deactivate the entire module
But I'm fine if we want to merge them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [johnzon] jungm commented on pull request #101: TCKs in github actions
Posted by "jungm (via GitHub)" <gi...@apache.org>.
jungm commented on PR #101:
URL: https://github.com/apache/johnzon/pull/101#issuecomment-1545190394
Is this PR even still needed then if we'll integrate running TCKs into core/jsonb modules? (and probably skip the failing ones for now?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@johnzon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org