If a test changes, poly test treats it as if the source changed

[seancorfield]

From Slack:

Q about poly test figuring out what changed and what needs to be tested: if I change just a test in a widely-used component, I would expect that component's tests to be re-run (for each project that uses it) but I wouldn't expect it to trigger a cascade of everything that depends on that component since the component hasn't changed, only one of its tests -- but it seems that everything (that depends on that component) ends up getting re-tested... which is complete overkill? Thoughts?

@tengstrand said:

I agree, this could be improved. I remember that I realised this weakness in the calculation, but that I thought it was so rare and that it simplified the calculations, so I decided not to act on it. Maybe you could create an issue, and I can have a look at it.

In the end, the solution to this issue was to include dependencies to bases from bases in the workspace structure and in the dependency calculations and the deps command, and to support exclusion of bricks in tests for projects in workspace.edn. Earlier we supported :include but now we also support :exclude.

[tengstrand]

This issue grew in size, but after several week of work, now it's time to test it @seancorfield. The documentation is not updated yet, so here comes a summary of the changes.

• Earlier, we only calculated if we depended from :src or :test. Now we also calculate if we depend on :src or :test. So if a base only depend on another base's :test code, and only its :src changes, then we only run tests for the changed base, not the depender.
• :src and :test have been added to :changes -> :changed-bases, :changed-components, and :changed-projects in the workspace structure.
• :src and :test have been added to :projects:PROJECT:deps in different places of the workspace structure.
• Old exported workspaces can still be used and switched to (with switch-ws) and will be converted to the new format and stored under :src.

If running the deps command with no arguments, it will work as today:

When giving a project, we no longer include indirect changes by default:

They can be included by passing in :indirect:

By giving both :src and :test, it will split up :src and :test separately (s = :src, t = :test, st = :src and :test):

If only :src is given, then only the dependencies from :src are shown:

If only :test is given, then only the dependencies from :test are shown:

If a brick is given, it will look as before, but the :src and :test will be shown:

If both brick and project is given, it will split up the dependencies in :src and :test:

Indirect dependencies can be included by passin in :indirect. Direct dependencies will be shown as s and t, indirect as s+ and t+:

If :src is passed in, then only dependencies from :src is shown:

If :test is passed in, then only dependencies from :test is shown:

[seancorfield]

That looks great. I'll be at Conj/traveling this week so I'll take a look when I get back.

[tengstrand]

Good to hear! Have a nice trip and stay.

[seancorfield]

Testing this now. When I first try check, I get this (which is expected with the new logic):

  Error 107: Missing bases in the wsbilling-rebill project, for the test context: billing-member

And when I move the billing-member base dep from the :test deps on the billing-rebill base to the wsbilling-rebill project, it checks "OK". So that's good.

When I run test, it tests the wsbilling-rebill project and tests everything it depends on (since deps.edn has changed), including the billing-member base -- which I think is expected this first time.

I'll see how this works for a few days and report back.

[seancorfield]

OK, I'm stuck again. Because that runs both the billing-rebill tests and then runs the billing-member tests as part of that, the tests fail -- because the billing-member tests can't be run in the same project context as the billing-rebill tests (and they aren't, when they are a :test dep of the base using Polylith master).

So that seems to suggest the test selection is wrong here?

[tengstrand]

Okay. Can you export the workspace again and send me @seancorfield , using the latest commit in the issue-205 branch (sha d509a1f04824a8d20fb5964d67f976a19e8a9246). The issue-205 branch continue from issue-274, and contains the latest and greatest. I need you to export the workspace again, because now I also store target :src and :test.

[tengstrand]

I have created the example workspace cross-ref in the issue-205 branch and managed to reproduce the problem.

The workspace looks like this:

Only the test context of the bm base is included in the brp project.

The br base refers to bm from its test context, which can be displayed with deps project:brp :src :test:

...or with ws get:projects:brp:deps

If the :src context of br changes, only its own tests will be executed (from the brp project) which is correct:

If the :test context of br changes, only its own tests will be executed (from the brp project) which is correct:

If the :src context of bm changes, the tests for bm will be executed (from the brp project) which is not correct. The bug is that the tool dosn't take into account that only the test context of bm is included in brp:

Will have a look at this!

[tengstrand]

You can try out the issue-205 branch, sha e41b1a4264c4b568943bfb242b488b660556aab2 @seancorfield.

[seancorfield]

This still runs the billing-member base tests when only billing-rebill (base) and wsbilling-rebill (project) have changed -- which breaks things:

Projects to run tests from: wsbilling-rebill

Running tests for the wsbilling-rebill project using test runner: Polylith org.corfield.external-test-runner...
Running tests from the wsbilling-rebill project, including 93 bricks: a-b-tester, admin-purchase-data, affiliate, application-fixtures, 
application-system, appstoreconnect, billing-actions, billing-android, billing-authorizenet, billing-braintree, billing-core, 
billing-data, billing-dmz, billing-fixtures, billing-google, billing-i18n, billing-machine, billing-member-ref, billing-notifications, 
billing-payment-method, billing-protocols, billing-rebill-actions, billing-sdk, billing-shared, billing-specs, billing-upkeep, 
caching, command-member-profile, configuration, connections, covalence-delivery, covalence-disposition, currency, 
datasources, date-time, environment, error-reporter, event-publisher, exceptions, executors, feature-flags, file-system, 
geo-location, google-measurement, host-services, http-client-hato, http-client-response, i18n, ip6, jwt, legacy-membership, 
logging, mail-system, mail-template, mailer, member-activity, member-id, member-interactions, member-profile-fields, 
messaging-sdk, money, newrelic, notification-settings, notifications, oauth-resource, oauth-tokens, onesignal, 
packages-and-rates, photo-data, photo-files, profile-template, profile-updates, push-notifications, query-member-profile, 
query-shared-photo, redis, safe-coercions, secure-token, selmer, site, slack-notifications, string-utils, system-properties, 
tagging, user-attributes, uuid, value-lists, web-middleware, web-request, web-server, with-retry, 
billing-member, billing-rebill
   ^^^^ this is a base and it has not changed so its tests should not be run here
Running test setup for the wsbilling-rebill project: ws.application-fixtures.interface/pre-test

[tengstrand]

Okay, will have another look. Also, I just rebased the issue-205 branch, so the equivalent sha is now 73a47eec8cd7933a511ba8401e25f37b1364bf93 (with the same problem as you just reported).

[tengstrand]

if no one succeeds in convincing me otherwise, I intend to skip keeping track of whether one depends on :src or :test, even though I have spent a lot of time building support for it in the 274/205 branches. Normally, both :src and :test code depend on :src only. The problem you have reported here @seancorfield, can be solved by adding support for :exclude in workspace.edn, under the :test key for each project (as I wrote in our Slack conversation). This solution has added a lot of complexity and it gives very little value back.

[seancorfield]

I agree. It seems like all this extra tracking stuff is really complex -- and I think it has really slowed down the poly tool on our repo (that's subjective: the latest branches/SHAs I've tried feel a lot slower than the current master version).

Being able to :exclude specific bricks from testing for certain projects would definitely address the edge case we have, so I'd be happy with that.

[seancorfield]

Confirmed the slowdown from this extra tracking is close to 25% on our repo, FYI, with clojure -M:poly check taking about 44 seconds with the issue-205 branch on average vs about 35 for master.

[tengstrand]

Maybe I'll give it a try to simplify the calculations instead.

[seancorfield]

You mean in the core tracking that is already done on master, right?

[tengstrand]

Yes, simplify the dependency calculations that is in master.

[tengstrand]

Now you should be able to exclude billing-member from wsbilling-rebill in workspace.edn, like this:

            "wsbilling-rebill" {:alias "br",
                                :exclude ["billing-member"]
                                :test {:setup-fn ws.application-fixtures.interface/pre-test,
                                       :teardown-fn ws.application-fixtures.interface/post-test}},

Please try it out @seancorfield from the issue-274 branch, e993ba78a2571f367edbe975431fd7d8d5ad9af1.

[seancorfield]

And, just to check, this new branch also has the slower dependency calculations? (based on my quick timings locally)

[seancorfield]

Looking at the change, the :exclude needs to go inside :test...

[seancorfield]

OK, with :exclude inside :test, it seems to work -- thank you! -- so now it's just a matter of simplifying the dependency tracking (or perhaps just removing the separate src/test tracking and going back to just the base dependency addition)?

[tengstrand]

Okay, great. It's really strange that this is slower to you compared to master, because now the issue-274 branch only includes the support for :exclude + a few minor refactorings (not the split between src/test).

I added issue #309

[seancorfield]

I was comparing against 01a0c9d7783ac2ac9abab4d7d3d0fe4e28ffdb4e which was much faster than current master. So it's something that got merged since then that has slowed things down. I can check each of the commits to master if you want?

[tengstrand]

Sure, please do that.

[seancorfield]

With 01a0c9d7783ac2ac9abab4d7d3d0fe4e28ffdb4e (what I was testing against as a baseline):

(/var/www/worldsingles)-(!2013)-> for t in 1 2 3; do time clojure -M:poly check; done
OK

real    0m25.974s
user    0m30.182s
sys     0m1.034s
OK

real    0m26.232s
user    0m30.533s
sys     0m1.108s
OK

real    0m27.111s
user    0m31.382s
sys     0m1.172s

Then e4d3f0e67bf1082d686b3049682b3f811cb1925a:

(/var/www/worldsingles)-(!2013)-> for t in 1 2 3; do time clojure -M:poly check; done
OK

real    0m30.039s
user    0m37.695s
sys     0m1.475s
OK

real    0m30.802s
user    0m36.276s
sys     0m1.304s
OK

real    0m29.589s
user    0m34.693s
sys     0m1.202s

So that's quite a big slowdown.

Then bd84789c9fe41eaaf19d431f1fc8772808c9eaad:

(/var/www/worldsingles)-(!2013)-> for t in 1 2 3; do time clojure -M:poly check; done
  Error 107: Missing components in the wsbilling-rebill project, for the test context, for these interfaces: billing-member

real    0m34.537s
user    0m45.619s
sys     0m1.759s
  Error 107: Missing components in the wsbilling-rebill project, for the test context, for these interfaces: billing-member

real    0m33.483s
user    0m41.494s
sys     0m1.411s
  Error 107: Missing components in the wsbilling-rebill project, for the test context, for these interfaces: billing-member

real    0m34.704s
user    0m43.606s
sys     0m1.444s

Another big slowdown. 10s in real time, nearly 15s in user time, compared to my baseline.

[tengstrand]

In e4d3f0e67bf1082d686b3049682b3f811cb1925a we started to read the source code using Edamame. In bd84789c9fe41eaaf19d431f1fc8772808c9eaad we started to calculate dependencies to other base.

I have added measurement of execution time for different parts of the poly tool in branch issue-274. I want you @seancorfield to run the tool using the latest sha in that branch, b6eb018715fe6e4789d2f41336c03a9f3e47182e.

Open a shell and run the tap command and then the check command. You can also run tap clear before you run check, to clear the tap window. For the polylith workspace, the output looks like this:

"#read-workspace, elapsed time: 0.375640542 seconds"
"#with-changes, elapsed time: 0.070522084 seconds"
"#enrich-workspace, elapsed time: 0.088930042 seconds"
"#enrich-projects, elapsed time: 0.074417041 seconds"
"#workspace-from-disk, elapsed time: 0.21611575 seconds"
"#read-projects, elapsed time: 0.007213917 seconds"
"#read-bases, elapsed time: 0.001894459 seconds"
"#read-components, elapsed time: 0.130565125 seconds"

Post the result in a comment here, and I will have a look (I'm on vacation next week, but will have a look when I get time).

[seancorfield]

"#read-workspace, elapsed time: 18.194704958 seconds"
"#with-changes, elapsed time: 0.281075637 seconds"
"#enrich-workspace, elapsed time: 14.747228914 seconds"
"#enrich-projects, elapsed time: 14.581228245 seconds"
"#workspace-from-disk, elapsed time: 3.16617327 seconds"
"#read-projects, elapsed time: 0.081658609 seconds"
"#read-bases, elapsed time: 1.321837468 seconds"
"#read-components, elapsed time: 1.448978695 seconds"

[tengstrand]

Solved by the issue-309 branch (issue #309) which was based on the issue-274 branch.

[tengstrand]

In the end, this issue was solved by handling dependencies from bases to other bases correctly and by adding support for excluding bricks to test in workspace.edn, e.g.:

{...
 :projects {...
            "command-line" {:alias "cl", :test {:exclude ["cli" "user"]}}}