Skip to content

Add a smoke test in build pipeline #355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 35 commits into from
May 20, 2022
Merged

Conversation

hossam-nasr
Copy link
Contributor

@hossam-nasr hossam-nasr commented Apr 27, 2022

Resolves #353 (per suggestion in additional info), related to #354 and #356.

I've never used Azure Pipelines (or even YAML files at all) before, so I'm not sure if this is the right or the most efficient way to do this, but at least it works 😛

Description

This new build job essentially replicates the repro steps in #353:

  1. builds and packs durable-functions into a .tgz file
  2. copies the samples/UnitTesting directory into a test-app directory, outside the durable SDK directory.
    • I chose to create the test app by copying the samples/UnitTesting directory, but if we make the whole directory runnable (see discussions here: Metadata generation failed #334 (comment) -- I've also created an issue here to track this: Make the samples directory easily runnable  #356), then we could instead copy the whole samples directory:
      • I didn't want to create the test app from scratch (mostly because it seemed more complicated) and preferred to just copy from the samples.
      • This was the only sample that was a fully working function app not just a sample function
      • I discovered later there are benefits to copying our samples (see below).
  3. npm installs the generated .tgz file from step 1 in the test app
  4. runs npm run build on the test app

Validation

Before merging #354 into this branch, the build pipeline failed (as expected) with this error (see here):

node_modules/durable-functions/lib/src/task.d.ts(4,25): error TS2307: Cannot find module 'moment' or its corresponding type declarations.

After merging, the build pipeline succeeds (see here)

Benefits

Besides checking for these kinds of type exporting mistakes in the future, while testing out this new pipeline, I realized there are other kinds of errors that can also be detected by this addition! I have fixed some of them in this PR too:

  • If we break a previous types contract, but is not evident from our own code. For example, all arguments to DummyOrchestrationContext were supposed to be optional, but I didn't realize that I broke this contract with Support Long Timers ⏳📈🚀🚀🚀 #340. It didn't cause any errors because none of our own code relied on the fact that those arguments are optional. Meanwhile, the samples/UnitTesting directory inside the durable directory referenced types from the npm durable-functions module, not from the local build. So this was only revealed when doing this kind of test.
  • If some of our sample code is not runnable (related to Make the samples directory easily runnable  #356). For example, the samples/UnitTesting app didn't import the mocha module, even though it referenced it. This also wasn't detected before, because mocha is imported by the root durable SDK directory.

Copy link
Collaborator

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some small suggestions. I'll defer to @ejizba to for the final approval here, but I'd love to hear your thought son my suggestions below.

@hossam-nasr hossam-nasr changed the title Add a check for exported types in build pipeline Add a smoke test in build pipeline Apr 28, 2022
Copy link
Collaborator

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hossam-nasr hossam-nasr force-pushed the hossamnasr/build-pipeline branch from 3af0d7e to 38d568f Compare May 18, 2022 00:19
@davidmrdavid
Copy link
Collaborator

@hossam-nasr: please let us know if this is ready for a re-review by requesting a new review through the UI from eric and me. Thanks

Copy link
Collaborator

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Waiting for @ejizba for the final word.

Copy link
Contributor

@ejizba ejizba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@hossam-nasr hossam-nasr merged commit 87b42f7 into dev May 20, 2022
@hossam-nasr hossam-nasr deleted the hossamnasr/build-pipeline branch May 20, 2022 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript build fails due to missing moment types
3 participants