Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

Presubmit does not error on build failures with current 11ty dependency #7513

Closed
jeffposnick opened this issue Mar 16, 2022 · 1 comment · Fixed by #7522
Closed

Presubmit does not error on build failures with current 11ty dependency #7513

jeffposnick opened this issue Mar 16, 2022 · 1 comment · Fixed by #7522
Assignees
Labels
eng - bug Something broke! eng For items that require engineering work. P2 A normal priority task. This is the default for most issues.

Comments

@jeffposnick
Copy link
Contributor

jeffposnick commented Mar 16, 2022

In the course of investigating #7512, I saw the following in our presubmit GH Action:

Unhandled rejection in promise ([object Promise]): (more in DEBUG output)
> Could not find i18n result for: undefined
    Error: Could not find i18n result for: undefined
        at Context.i18n (/home/runner/work/web.dev/web.dev/src/site/_filters/i18n.js:79:9)
        at Context.<anonymous> (/home/runner/work/web.dev/web.dev/node_modules/@11ty/eleventy/src/BenchmarkGroup.js:30:26)
        at eval (eval at _compile (/home/runner/work/web.dev/web.dev/node_modules/nunjucks/src/environment.js:[63](https://github.com/GoogleChrome/web.dev/runs/5569955160?check_suite_focus=true#step:4:63)3:18), <anonymous>:92:55)
        at /home/runner/work/web.dev/web.dev/node_modules/@11ty/eleventy-plugin-rss/.eleventy.js:21:7
        at runMicrotasks (<anonymous>)
        at processTicksAndRejections (node:internal/process/task_queues:96:5)

`Error` was thrown:

> web.dev@3.0.0 gulp:default-locale
> npx gulp default-locale

[12:36:47] Using gulpfile ~/work/web.dev/web.dev/gulpfile.js
[12:36:47] Starting 'default-locale'...
[12:36:47] Finished 'default-locale' after 17 ms

> web.dev@3.0.0 firebase-config
> node firebase-config.js

That is to say, the eleventy step of our npm-run-all sequence encountered an error, but instead of causing the process running that script to exit with a non-0 code, the error did not stop execution. The next step, gulp:default-locale, was run instead. This led to the overall process to exit with a code of 0, indicating success.

This behavior seemed strange to me, and I think it can be traced back to the version of 11ty we're currently using: v0.12.1. It includes the following:

  process.on("unhandledRejection", (error, promise) => {
    EleventyErrorHandler.error(
      error,
      `Unhandled rejection in promise (${promise})`
    );
  });

I.e. if there's an error thrown during an async part of the 11ty build, there's logging via EleventyErrorHandler.error(), but nothing will trigger a non-0 exit code.

More recent versions of 11ty have changed this behavior to something more in keeping with what you'd expect:

  process.on("unhandledRejection", (error, promise) => {
    errorHandler.fatal(error, "Unhandled rejection in promise");
  });

errorHandler.fatal() will log the message and set process.exitCode = 1.

We've been holding off on updating out 11ty dependency until the performance improvements from 11ty/eleventy#2214 are released (planned for v1.0.1), and hopefully that will be soon?

If for some reason, that release is massively delayed, then we should consider updating to v1.0.0 just to get the better error handling behavior.

@jeffposnick jeffposnick added P2 A normal priority task. This is the default for most issues. eng - bug Something broke! eng For items that require engineering work. labels Mar 16, 2022
@jeffposnick jeffposnick self-assigned this Mar 16, 2022
@jeffposnick
Copy link
Contributor Author

To close the loop on this, #7531 was a test with invalid author metadata, and now that we've updated to the latest 11ty, the presubmit check does indeed fail for this type of error.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
eng - bug Something broke! eng For items that require engineering work. P2 A normal priority task. This is the default for most issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant