search

eclipse / microprofile-metrics

microprofile-metrics
Apache License 2.0
100 stars 66 forks source link

Make testGcCountMetrics and testGcTimeMetrics more robust #722

Closed jgallimore closed 1 year ago

jgallimore commented 1 year ago

These tests split the prometheus output on a space, which means that the test breaks if the name tag has a space in it, which it does in the case where the G1 GC is used.

eclipse-microprofile-bot commented 1 year ago

Can one of the admins verify this patch?

Channyboy commented 1 year ago

ok to test

Channyboy commented 1 year ago

@jgallimore Works for me! I also noticed that the JSON commit you're basing off of is a little out of date (that's my fault, I probably force pushed)

https://github.com/eclipse/microprofile-metrics/pull/709 Has two commits: 92d9a2e7e3d0ff22a721e54cb6c3102cb6d9fb63 faf1758421bc63ec65eb5f98400730842f69f49f

The JSON earlier one referenced here is: 874fbf3c806801222ec949db0b56cd3a239f1223

It doesn't fix the space issue addressed here. But there is 1 additional unrelated test :b

jgallimore commented 1 year ago

@Channyboy I had a load of merge conflicts when I tried to open this PR, so its pretty likely I based it off something that wasn't up to date. I'll take another swing at it, and I'll try and correct the CLA issue as well.

jgallimore commented 1 year ago

@Channyboy I forced pushed this, based off commit https://github.com/eclipse/microprofile-metrics/commit/faf1758421bc63ec65eb5f98400730842f69f49f in #709 and fixed the author on the commit as well. This is hopefully now all ok, if you're able to re-review.

Channyboy commented 1 year ago

@jgallimore Had to rebase https://github.com/eclipse/microprofile-metrics/pull/709 again after a few PRs had been merged. But, otherwise, same as before, after cherry-picking your commits on top this runs fine.

Will approve after #709 goes in. just in case this gets accidentally merged before #709

jgallimore commented 1 year ago

I've resolved the merge conflicts - hopefully this is now good to go.