| [21:59:15] | <symbioquine[m]> | It looks like there's a race condition in how the Drupal test scaffolding deal with directory creation; https://github.com/farmOS/farmOS/pull/378#issuecomment-733410225 |
| [21:59:46] | <symbioquine[m]> | * It looks like there's a race condition in how the Drupal test scaffolding deals with directory creation; https://github.com/farmOS/farmOS/pull/378#issuecomment-733410225 |
| [22:02:01] | <symbioquine[m]> | With a rudimentary patch in place, the phpunit test run takes about 1 min 35 seconds on the Github build servers. |
| [22:03:58] | <symbioquine[m]> | ACTION uploaded an image: image.png (80KiB) < https://matrix.org/_matrix/media/r0/download/matrix.org/ROlSPWgOXuYALWDR... > |
| [22:07:28] | <symbioquine[m]> | In related news, the paratest changes + [this guidance](https://shusson.info/post/non-durable-postgres-for-e2e-tests) tuning postgres yields ~20s test run times on my local (admittedly beefy) system. |
| [22:08:11] | <symbioquine[m]> | ACTION uploaded an image: image.png (15KiB) < https://matrix.org/_matrix/media/r0/download/matrix.org/sHLYTTRPbqhpNTid... > |
| [22:13:24] | <symbioquine[m]> | ACTION sent a long message: < https://matrix.org/_matrix/media/r0/download/matrix.org/zQRrlnwzbyZUVYDz... > |
| [22:15:33] | <symbioquine[m]> | Important Note: The above change should only be used for testing - not production - since it makes Postgres writes potentially non-durable! :) |
| [06:03:50] | <mstenta[m]> | WHOA |
| [06:04:07] | <mstenta[m]> | Man thanks for digging into these optimizations symbioquine ! It will make rapid testing much easier! |
| [06:45:18] | <mstenta[m]> | Just looking at `docker-compose.testing.yml` again - maybe we should add a few brief comments to explain that change, as well as the `test-runner` `command` line |
| [06:45:52] | <mstenta[m]> | Could figure it out by digging into `git blame` and finding the original issue, but a comment would be quicker :-) |
| [06:46:18] | <mstenta[m]> | Thinking specifically about the need for `sleep` |
| [06:57:10] | <mstenta[m]> | Commented on all the thread... :-) |
| [06:57:14] | <mstenta[m]> | * Commented on all the threads... :-) |
| [08:07:43] | <mstenta[m]> | symbioquine: hmm we're seeing some new test failures... not sure what's causing them... |
| [08:07:43] | <mstenta[m]> | https://github.com/mstenta/farmOS/runs/1453513882 |
| [08:07:47] | <mstenta[m]> | in 2.x |
| [08:09:01] | <mstenta[m]> | Or rather... not failures... deprecation notices (but causes the action to show as failed) |
| [08:10:11] | <mstenta[m]> | I don't think it's a result of any of our changes - I pushed `2.x` last night and it worked - seems to have started this morning |
| [08:10:15] | <mstenta[m]> | So maybe an upstream change? |
| [08:11:09] | <mstenta[m]> | (We've seen upstream changes cause failures like this already. eg: new default codesniffer rules enabled upstream that cause our code sniffer step to fail) |
| [10:53:37] | <paul121[m]> | mstenta: those failures may be due upstream changes in the `simple_oauth` library. Specifically this commit: https://git.drupalcode.org/project/simple_oauth/-/commit/a994df6fd5fb282... |
| [10:53:37] | <symbioquine[m]> | Ah! That explains it... I was seeing those too, but couldn't figure out how I had caused them... |
| [10:54:20] | <symbioquine[m]> | https://github.com/symbioquine/farmOS/runs/1451535694?check_suite_focus=... |
| [10:55:05] | <mstenta[m]> | Hmm but why would it just start happening today? |
| [10:55:32] | <paul121[m]> | I know... not sure |
| [10:56:05] | <mstenta[m]> | symbioquine: yea I didn't think it was your changes either, because I pushed a commit last night that did not fail |
| [10:56:23] | <symbioquine[m]> | <mstenta[m] "Hmm but why would it just start "> I was thinking about this problem recently... |
| [10:56:40] | <symbioquine[m]> | I think it's because the composer.lock file isn't being checked in. |
| [10:56:48] | <mstenta[m]> | Yea |
| [10:57:01] | <mstenta[m]> | I think that's definitely part of it |
| [10:57:12] | <symbioquine[m]> | Each time builds run you get a whole new roll-of-the-dice set of dependency versions :) |
| [10:57:16] | <mstenta[m]> | > (We've seen upstream changes cause failures like this already. eg: new default codesniffer rules enabled upstream that cause our code sniffer step to fail) |
| [10:57:21] | <mstenta[m]> | Yup |
| [10:57:26] | <mstenta[m]> | paul121 and I have talked about that |
| [10:57:37] | <mstenta[m]> | We should commit `composer.lock` eventually |
| [10:57:50] | <mstenta[m]> | But we figured it wouldn't hurt to be "bleeding edge" right now :-) |
| [10:57:56] | <mstenta[m]> | Just means we need to fix these kinds of things more often |
| [10:58:04] | <symbioquine[m]> | Yeah, it has its advantages too |
| [10:58:20] | <mstenta[m]> | As we get closer to 2.0.0 we'll definitely want to commit `composer.lock` though - agreed |
| [10:59:15] | <mstenta[m]> | paul121 had the idea to set up a "nightly" test that works without `composer.lock` as well... so we can be aware of these things, without necessarily being blocked by them |
| [10:59:38] | <mstenta[m]> | Worth noting: the tests are not FAILING |
| [10:59:49] | <mstenta[m]> | they're just returning an exit code 1 - because of the deprecation notices I think |
| [11:00:24] | <mstenta[m]> | And looks like all the notices are in `farm_api` - so we can probably just fix them ourselves |
| [11:00:32] | <paul121[m]> | if `simple_oauth` is at fault, why didn't we see this earlier? That commit was released in 5.0.1 almost two months ago? And we are pinned at `^5.0` ? |
| [11:01:01] | <mstenta[m]> | I don't think it's `simple_oauth` |
| [11:01:34] | <mstenta[m]> | I think probably some things were just deprecated in the testing infrastrucutre upstream |
| [11:01:39] | <mstenta[m]> | Which weren't deprecated before |
| [11:01:45] | <mstenta[m]> | So now we're seeing warnings |
| [11:01:49] | <paul121[m]> | ahh okay |
| [11:02:03] | <mstenta[m]> | Looks like simple fixes in the `farm_api` module will fix it |
| [11:02:38] | <paul121[m]> | ahh well... some of those tests inherit from `simple_oauth` |
| [11:02:44] | <mstenta[m]> | oooh :-( |
| [11:02:47] | <mstenta[m]> | doh |
| [11:02:56] | <mstenta[m]> | time to open a d.o issue i guess :-/ |
| [11:03:14] | <mstenta[m]> | (in https://drupal.org/project/simple_oauth) |
| [11:03:29] | <paul121[m]> | yea. this is what caught us last time: https://www.drupal.org/project/simple_oauth/issues/3174572 |
| [11:04:26] | <paul121[m]> | I started investigating converting these to Kernel tests, which would NOT inherit from `simple_oauth` |
| [11:04:42] | <mstenta[m]> | hmm - well... should we maybe temporarily move `farm_api` tests to a `farm_api` group - and stop running them in GHActions until it's fixed? |
| [11:04:43] | <paul121[m]> | it was mostly convenience to inherit from their test base |
| [11:04:50] | <mstenta[m]> | oh ok |
| [11:04:56] | <mstenta[m]> | well if that looks like a good way forward... |
| [11:05:25] | <paul121[m]> | but yea, I'm not sure if its an easy way forward... haha |
| [11:05:42] | <mstenta[m]> | ok |
| [11:06:51] | <mstenta[m]> | would it be easier to patch `simple_oauth`? |
| [11:07:19] | <paul121[m]> | yea I think so |
| [11:07:55] | <paul121[m]> | we consider refactoring our Functional tests to NOT inherit from `simple_oauth` |
| [11:08:21] | <paul121[m]> | but then I think we'd need to re-create 90% of the `setUp()` that their base class does anyways |
| [11:08:26] | <mstenta[m]> | right |
| [11:08:42] | <mstenta[m]> | seems like contributing a patch is all around easiest and best for upstream too |
| [11:08:59] | <mstenta[m]> | e0ipso would need to fix it eventually anyway :-) |
| [11:10:00] | <paul121[m]> | yep! |
| [11:10:20] | <mstenta[m]> | symbioquine: ok if we close this PR? https://github.com/farmOS/farmOS/pull/285 |
| [11:10:36] | <mstenta[m]> | maybe worth looking into re-implementing on 2.x! |
| [11:11:44] | <mstenta[m]> | and on that note... maybe we want to consider adding this: https://www.drupal.org/project/key_value_field |
| [11:12:12] | <symbioquine[m]> | <mstenta[m] "symbioquine: ok if we close this"> Sounds good |
| [11:12:32] | <mstenta[m]> | On another topic... |
| [11:12:45] | <mstenta[m]> | I merged the `CONTRIBUTING.md` change, but it's not appearing at the top of hte issue queue :-( |
| [11:13:48] | <mstenta[m]> | I wonder if something else is required |
| [11:13:50] | <mstenta[m]> | Oh also... I found a neat feature! |
| [11:14:00] | <mstenta[m]> | Check out the `contact_links` here: https://github.com/GenericMappingTools/gmt/blob/master/.github/ISSUE_TEM... |
| [11:14:09] | <mstenta[m]> | Results in: https://github.com/GenericMappingTools/gmt/issues/new/choose |
| [11:14:14] | <mstenta[m]> | We can link to our forum that way! |
| [11:15:38] | <paul121[m]> | really cool! |
| [11:15:46] | <symbioquine[m]> | ACTION uploaded an image: image.png (62KiB) < https://matrix.org/_matrix/media/r0/download/matrix.org/lrqRSMBAsMztIkSP... > |
| [11:15:49] | <mstenta[m]> | I'll give it a go... |
| [11:16:19] | <mstenta[m]> | Oh yes... the `ISSUE_TEMPLATE` works... but I was hoping to see something like this with the addition of `CONTRIBUTING.md`: |
| [11:16:20] | <symbioquine[m]> | ACTION uploaded an image: image.png (127KiB) < https://matrix.org/_matrix/media/r0/download/matrix.org/seJdtcSeOWsqqgML... > |
| [11:16:48] | <mstenta[m]> | ACTION uploaded an image: Screenshot from 2020-11-25 11-16-38.png (78KiB) < https://matrix.org/_matrix/media/r0/download/matrix.org/bmiloXakEEvLwrEY... from 2020-11-25 11-16-38.png > |
| [11:19:55] | <mstenta[m]> | ACTION uploaded an image: Screenshot from 2020-11-25 11-19-32.png (30KiB) < https://matrix.org/_matrix/media/r0/download/matrix.org/UItPvSuAeXZOpXDq... from 2020-11-25 11-19-32.png > |
| [11:19:55] | <mstenta[m]> | Neat! |
| [11:23:00] | <mstenta[m]> | Should I add a link to this chat there as well? |
| [11:23:41] | <paul121[m]> | Doesn't hurt |
| [11:39:28] | <mstenta[m]> | I feel like we could probably close https://github.com/farmOS/farmOS/issues/374 once I figure out the `CONTRIBUTING.md` link - what do you think symbioquine paul121 |
| [11:39:53] | <mstenta[m]> | I like where we've landed on it - and we WILL keep it, so I think the question has been answered |
| [11:40:13] | <symbioquine[m]> | Makes sense to me |
| [11:40:49] | <mstenta[m]> | The issue templates alone will go a long way to directing newcomers to the forum and chat, and focus the GitHub issues on bug reports |
| [11:41:04] | <mstenta[m]> | We can leave current issues as-is, but slowly wind them down moving forward |
| [11:48:34] | <mstenta[m]> | A nice "spring cleaning" activity :-) |
| [12:10:07] | <paul121[m]> | so one of those deprecation notices seems to have been pretty popular got noticed pretty quickly: https://github.com/tymondesigns/jwt-auth/issues/2059#issuecomment-733622082 |
| [12:10:19] | <paul121[m]> | * so one of those deprecation notices seems to have been pretty popular & got noticed pretty quickly: https://github.com/tymondesigns/jwt-auth/issues/2059#issuecomment-733622082 |
| [12:10:51] | <mstenta[m]> | ah |
| [12:10:56] | <mstenta[m]> | upupstream |
| [12:11:16] | <paul121[m]> | yes haha |
| [12:14:56] | <symbioquine[m]> | <mstenta[m] "hmm - well... should we maybe te"> It seems like it would be nice to be able to ignore specific instances of warnings. i.e. ignore E_WARNINGs generated on this line(s) of these file(s). That way the test can still be kept to protect against other regressions while acknowledging that some warnings are expected until they get fixed upstream... |
| [12:15:38] | <mstenta[m]> | hmm yea... thought about that too... although we would want to see deprecation notices for our own code i think |
| [12:15:44] | <mstenta[m]> | so might be hard to separate |
| [12:15:45] | <symbioquine[m]> | It might be possible to do something like that with a decorated error handler function. |
| [12:16:32] | <symbioquine[m]> | For simplicity, the "configuration" of which to ignore could just be hard-coded into the function until it gets too unweildy. |
| [12:16:49] | <symbioquine[m]> | * For simplicity, the "configuration" of which to ignore could just be hard-coded into the function until it gets too unwieldy. |
| [12:18:10] | <paul121[m]> | maybe we ignore E_WARNINGs on all tests except for the nightly |
| [12:19:24] | <mstenta[m]> | yea that could work |
| [12:19:55] | <mstenta[m]> | would that require two nearly identical `run-test.yml` files though? |
| [12:20:15] | <mstenta[m]> | or maybe the logic to turn that off could be in the same one |
| [12:20:32] | <mstenta[m]> | i don't know what the mechanism is for turning those warnings off, so i can't really say how |
| [12:21:40] | <paul121[m]> | I think ALL of these deprecation notices were caused by the up-upstream `/lcobucci/jwt`: https://github.com/lcobucci/jwt/issues/550#issuecomment-733557709 |
| [12:22:53] | <mstenta[m]> | ah ok - so they need to be fixed there? |
| [12:23:08] | <mstenta[m]> | are they working on that? maybe this will go away as easily as it appeared? :-) |
| [12:23:58] | <paul121[m]> | no, its the approach they used to trigger deprecation notices in a new release (3.3.4) |
| [12:24:18] | <paul121[m]> | LOTS of tests are failing this morning with the same thing we are seeing :-) |
| [12:24:27] | <mstenta[m]> | oh ok haha |
| [12:24:45] | <paul121[m]> | so... pinning that to 3.3.3 should fix. I have a test running rn... |
| [12:25:15] | <mstenta[m]> | cool! is that in `simple_oauth`? |
| [12:26:31] | <paul121[m]> | no... simple_oauth uses thephpleague/oauth2-server and thephpleague/oauth2-server uses lcobucci/jwt |
| [12:26:44] | <mstenta[m]> | oh ok |
| [12:27:05] | <mstenta[m]> | are they working on a fix already in `thephpleague/oauth2-server`? |
| [12:28:00] | <mstenta[m]> | oh found it |
| [12:28:01] | <mstenta[m]> | https://github.com/thephpleague/oauth2-server/issues/1156 |
| [12:28:06] | <paul121[m]> | yes I think so: |
| [12:28:06] | <paul121[m]> | > We worked together with both Passport and Oauth2-server maintainers to make sure everything is properly tested and gets released (they were just waiting for me to release things). |
| [12:28:07] | <mstenta[m]> | https://github.com/thephpleague/oauth2-server/pull/1146 |
| [12:34:10] | <paul121[m]> | Tests should pass for this... https://github.com/paul121/farmOS/commit/3d056d9f55e3041e217cab42465846d... |
| [12:35:49] | <symbioquine[m]> | <mstenta[m] "i don't know what the mechanism "> I was proposing something like this; |
| [12:36:15] | <symbioquine[m]> | Of course, Paul's solution is better for this case. |
| [12:36:26] | <mstenta[m]> | Ah gotcha! |
| [12:36:33] | <symbioquine[m]> | But in the future if there isn't an easy dependency fix... |
| [12:41:04] | <paul121[m]> | oh! theres actually a `$this->expectDeprecationMessage()` assertion for phpunit |
| [12:41:05] | <paul121[m]> | https://symfony.com/doc/current/components/phpunit_bridge.html#id1 |
| [12:43:30] | <paul121[m]> | that would allow individual tests to handle this, rather than a catch-all like you're suggesting. I imagine there are use cases for both! |
| [12:45:10] | <symbioquine[m]> | Yeah that's a better strategy :) |
| [12:45:39] | <paul121[m]> | > Tests should pass for this... https://github.com/paul121/farmOS/commit/3d056d9f55e3041e217cab42465846d... |
| [12:45:39] | <paul121[m]> | mstenta tests passed. just need to keep an eye on `thephpleague/oauth2_server` and make sure `simple_oauth` bumps the dependency sooner rather than later |
| [12:59:37] | <mstenta[m]> | Awesome can you add a comment to composer.json to make note of that? |
| [13:00:01] | <mstenta[m]> | symbioquine I saw you used some method of commenting that I hadn't seen before... |
| [13:00:25] | <paul121[m]> | yea, my editor got mad when I tried to add a comment to JSON... maybe there is a way? |
| [13:00:48] | <mstenta[m]> | Yea one sec on my phone |
| [13:01:00] | <mstenta[m]> | symbioquine s paratest PR |
| [13:01:17] | <mstenta[m]> | Oh but maybe the composer-project one |
| [13:01:55] | <mstenta[m]> | symbioquine does that print to the screen by any chance? That would be nice :-) |
| [13:02:29] | <mstenta[m]> | https://github.com/farmOS/composer-project/pull/1/files |
| [13:11:47] | <symbioquine[m]> | <mstenta[m] "symbioquine does that print to t"> I don't recall it doing so... |
| [13:13:25] | <symbioquine[m]> | Yeah, I don't think it prints anything related to that comment section during the composer install step. |
| [13:14:15] | <mstenta[m]> | Ah I guess we wouldn't typically see it anyway |
| [13:15:43] | <symbioquine[m]> | In case folks are curious, the most relevant discussion of how to comment in composer.json is at https://github.com/composer/composer/issues/5364#issuecomment-221220653 |
| [14:20:18] | * farmBOT has joined #farmos |
