Better dependency management, run tests against high and low version of deno #79

filmaj · 2023-05-05T16:29:14Z

Tweaks CI to run against most recent stable deno as well as a minimum deno version (1.31.1 - based on what version of deno we Run on Slack)
Use of deno stdlib should have a tightly controlled version based on the range of deno runtime versions we intended to support. This list (specifically the cli_to_std map) should be followed to determine what stdlib version to use.
Moved any generation script dependencies into their own file under the scripts/src directory, to ensure these dependencies are not pulled in to user applications.
Added comments to dependency files to try to clarify reasoning behind the choice of deno std version consumption.

codecov · 2023-05-08T14:25:11Z

Codecov Report

Merging #79 (d58caee) into main (a3664f6) will not change coverage.
The diff coverage is 100.00%.

❗ Current head d58caee differs from pull request most recent head 02e486b. Consider uploading reports for the commit 02e486b to get more accurate results

@@            Coverage Diff            @@
##              main       #79   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           35        35           
  Lines         1064      1074   +10     
  Branches        10        10           
=========================================
+ Hits          1064      1074   +10

Impacted Files	Coverage Δ
src/deps.ts	`100.00% <100.00%> (ø)`
src/dev_deps.ts	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…pts/src directory. Comment at the top of deps.ts and dev_deps.ts to guide what version of deno std we should use. run tests against most recent stable deno as well as minimum deno version used internally Move import map helper scripts (for use with sample app integration testing) under scripts/src, add comments to scripts/deps.ts to explain stdlib versioning strategy. Run both CI workflows against most recent stable deno as well as version of deno used by Run on Slack. Name deno CI workflow more specifically Set min deno version to 1.31.1 (version Run on Slack uses), set deno stdlib version to 0.178.0 (recommended for deno v1.31.1), update tests as a result of assertion method changes in 0.178.0.

filmaj · 2023-05-08T15:06:46Z

src/api_test.ts

@@ -71,19 +65,16 @@ Deno.test("SlackAPI class", async (t) => {
              });
            });

-            await assertRejects(


The API for assertRejects changed in a recent version of deno's stdlib, leading to type errors. So, removed its use and instead used a 'manual' try/catch approach.

filmaj · 2023-05-08T15:08:05Z

src/api_test.ts

+              await client.apiCall("chat.postMessage", {});
+            } catch (error) {
+              assertInstanceOf(error, HttpError);
+              if (isHttpError(error)) {


This conditional should always return true. If it doesn't, then the previous line's assertion would fail. Its main purpose in this block is for type narrowing; using isHttpError narrows the error type and allows for referencing e.g. headers and status on the error object.

nit: Might be good to have this context in a small in-code comment

filmaj · 2023-05-08T15:08:55Z

src/typed-method-types/workflows/triggers/inputs.ts

    ? [keyof Params["properties"]] extends [never] ? EmptyInputs
-    : Params["required"] extends Array<infer T> ? [T] extends [never]
-        // If there are no required properties, inputs are optional
+    : Params["required"] extends Array<infer T> ? [T] extends [never] // If there are no required properties, inputs are optional


These were a newer deno version's deno fmt changes.

WilliamBergamin

This is a clean solution to the script dependency management issue 🥇 I was struggling with this earlier and did not think it would affect us this quickly

Left one comment, let me know what you think about it.

src/api_test.ts

filmaj · 2023-05-08T18:14:45Z

src/typed-method-types/workflows/triggers/tests/event_test.ts

@@ -85,13 +85,10 @@ Deno.test("Mock call for event", async (t) => {
            },
          });
          assertEquals(res.ok, true);
-          if (res.ok) {


The assertion above this line should ensure res.ok is always true. As a result, this conditional is not needed. (similar change made to other similar tests in other files below)

filmaj · 2023-05-08T18:15:36Z

src/typed-method-types/workflows/triggers/tests/event_test.ts

@@ -85,13 +85,10 @@ Deno.test("Mock call for event", async (t) => {
            },
          });
          assertEquals(res.ok, true);
-          if (res.ok) {
-            assertEquals(res.trigger, event_response.trigger);


I found this assertion to not be very useful. Since we control the contents of the response via the mocking utility earlier in the test, it will always be true. As a result, I removed it.

WilliamBergamin

LGTM 🚀

shapirone

I know you want to revisit, but this LGTM as an iterative step (other than the ROSI Deno version)

shapirone · 2023-05-15T18:17:54Z

.github/workflows/deno.yml

+      matrix:
+        # we test on both most recent stable version of deno (v1.x) as well as
+        # the version of deno used by Run on Slack (v1.31.1)
+        deno-version: [v1.x, v1.31.1]


I believe this is already out of date with what we run on ROSI.

Is there a way we can sync this with what's on ROSI? Nothing to do here now but let's keep this in the back of our mind as we think about how to surface the ROSI Deno version.

Totally, we talked about getting ROSI to post their deno runtime version in metadata.json, so I imagine this CI script would pull from there and use that as the lower-bound deno version to test.

shapirone · 2023-05-15T18:19:35Z

scripts/src/generate.ts

+import { pascalCase } from "./deps.ts";
+import { emptyDir, ensureDir } from "./deps.ts";


nit: One line?

shapirone · 2023-05-15T18:23:09Z

src/api_test.ts

+              await client.apiCall("chat.postMessage", {});
+            } catch (error) {
+              assertInstanceOf(error, HttpError);
+              if (isHttpError(error)) {


nit: Might be good to have this context in a small in-code comment

shapirone · 2023-05-15T18:24:05Z

src/deps.ts

+// When changing std versions, ensure to check this list's `cli_to_std` section: https://raw.githubusercontent.com/denoland/dotland/main/versions.json
+// Whatever minimum deno version we are recommending to users should be used to index into the above list to determine the maximum version of std
+// we should use in our libraries.


Bonus points if we can figure out a way to do this automatically!

shapirone · 2023-05-15T18:38:07Z

src/dev_deps.ts

 export {
  afterEach,
  beforeAll,
-} from "https://deno.land/std@0.185.0/testing/bdd.ts";
+} from "https://deno.land/std@0.178.0/testing/bdd.ts";
+export { isHttpError } from "https://deno.land/std@0.178.0/http/http_errors.ts";


We can take this offline, but our libraries define the same version of the std library over and over making updates more challenging. I understand the "import maps are for applications" but using an import map would simplify this, and allow consuming developers to override the std lib via scopes 🤔

I wish imports and exports supported dynamic strings 😢

If we could use an import_map it could solve this 😢

filmaj self-assigned this May 5, 2023

filmaj force-pushed the deps-mgmt branch from c0251bc to 839317e Compare May 5, 2023 16:31

filmaj force-pushed the deps-mgmt branch from 6c55ca1 to cbdb943 Compare May 8, 2023 15:04

filmaj commented May 8, 2023

View reviewed changes

filmaj marked this pull request as ready for review May 8, 2023 15:45

filmaj requested a review from a team as a code owner May 8, 2023 15:45

filmaj changed the title ~~Deps mgmt~~ Better dependency management, run tests against high and low version of deno May 8, 2023

WilliamBergamin requested changes May 8, 2023

View reviewed changes

src/api_test.ts Show resolved Hide resolved

filmaj commented May 8, 2023

View reviewed changes

filmaj added 3 commits May 8, 2023 14:18

Ensure exception is thrown in a couple of API tests.

408c8eb

Small clarification on stdlib dependency versioning strategy.

26dd4a8

Small clarification on stdlib dependency versioning strategy.

d58caee

WilliamBergamin self-requested a review May 9, 2023 15:36

WilliamBergamin approved these changes May 11, 2023

View reviewed changes

filmaj requested a review from shapirone May 11, 2023 17:37

filmaj added enhancement New feature or request semver:patch requires a patch version number bump labels May 11, 2023

shapirone reviewed May 30, 2023

View reviewed changes

Added comments and minor tweaks that arose from review.

02e486b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better dependency management, run tests against high and low version of deno #79

Better dependency management, run tests against high and low version of deno #79

filmaj commented May 5, 2023 •

edited

codecov bot commented May 8, 2023 •

edited

filmaj May 8, 2023 •

edited

filmaj May 8, 2023 •

edited

shapirone May 15, 2023

filmaj May 8, 2023

WilliamBergamin left a comment

filmaj May 8, 2023

filmaj May 8, 2023

WilliamBergamin left a comment

shapirone left a comment

shapirone May 15, 2023

filmaj May 30, 2023

shapirone May 15, 2023

shapirone May 15, 2023

shapirone May 15, 2023

shapirone May 15, 2023

filmaj May 30, 2023

WilliamBergamin May 31, 2023

		import { pascalCase } from "./deps.ts";
		import { emptyDir, ensureDir } from "./deps.ts";

Better dependency management, run tests against high and low version of deno #79

Are you sure you want to change the base?

Better dependency management, run tests against high and low version of deno #79

Conversation

filmaj commented May 5, 2023 • edited

codecov bot commented May 8, 2023 • edited

Codecov Report

filmaj May 8, 2023 • edited

Choose a reason for hiding this comment

filmaj May 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamBergamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamBergamin left a comment

Choose a reason for hiding this comment

shapirone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj commented May 5, 2023 •

edited

codecov bot commented May 8, 2023 •

edited

filmaj May 8, 2023 •

edited

filmaj May 8, 2023 •

edited