Add info messages to inform developers of expected errors

[AgentHagu]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• Feature addition or enhancement
• Code maintenance
• DevOps
• Improve developer experience
• Others, please explain:
  Improve user experience

Overview of changes:
Fixes #2612

This PR adds info messages for developers to inform them of the errors they should expect to see during npm run test.

The PR also improves the error message shown. Previously, it showed something like:

This is very confusing for users, since it exposes the inner call stack of MarkBind and doesn't inform the user of where to look for this issue. The new error message addresses both of these issues:

The PR also adds some missing tests for includePanelProcessor.ts

Anything you'd like to highlight/discuss:
The developer guide has also been updated to inform developers of these messages and errors

Testing instructions:

1. Run npm run build:backend
2. Run npm run test
3. Verify that there are accompanying info messages to inform developers about the expected logged errors

Proposed commit message: (wrap lines at 72 characters)
Add info messages to inform developers of expected errors

Currently, there are some error messages that appear during
npm run test. These errors are expected to be logged due to
negative test cases involving includePanelProcessor.test.ts.

These error messages may be confusing to developers, as they may
be unaware that the errors are expected. To fix this, let's add info
messages to inform developers of any expected errors to be seen
during the test run.

Let's also update the error messages to only display the message,
rather than the entire error stack. Let's update the developer guide
to inform current and future developers to look out for these info
messages and to update them whenever they add similar negative
test cases.

---

Checklist: ☑️

• Updated the documentation for feature additions and enhancements
• Added tests for bug fixes or features
• Linked all related issues
• No unrelated changes

---

Reviewer checklist:

Indicate the SEMVER impact of the PR:

• Major (when you make incompatible API changes)
• Minor (when you add functionality in a backward compatible manner)
• Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

• To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

• what was the old feature that was made obsolete
• any replacement feature (if any), and
• how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

[AgentHagu]

Actually, another way to allow for errors to users and warnings to developers is to detect process.env.TEST_MODE and log accordingly.

Would this solution be better to keep the urgency of errors for users and hide these intended errors from developers during testing?

    if (process.env.TEST_MODE) {
      logger.warn(error);
    } else {
      logger.error(`${error.message}
      File: ${context.cwf}
      URL provided: ${node.attribs.src}
      
      Please check the \`src\` attribute in the popover element.
      Ensure it doesn't contain a URL (e.g., "http://www.example.com").`);
    }

[tlylt]

I don't think there's a need to make such fine differentiation between a user and a developer by adjusting the log level. We can certainly improve the logging, but doesn't have to reduce it to a warning.
If I remember correctly this error will show up in the user's generated site due to cheerio(node).replaceWith(createErrorNode(node, error));. It's a serious issue that need the user's attention and rectification.

[AgentHagu]

Would it be better to instead log it as a success during TEST MODE? So we leave it as an error for both users and developers, but only during a test run do we adjust it to show something like "Success: invalid case of URL in popover element caught". This way, both users and developers will be aware of any issues since its still an error for them but testing will become less misleading. This fix can also be applied to the other error messages in includePanelProcessor.ts

For users and developers:

During testing (npm run test):

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.60%. Comparing base (da87da1) to head (76f82f3).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2639      +/-   ##
==========================================
+ Coverage   52.80%   53.60%   +0.79%     
==========================================
  Files         130      130              
  Lines        7162     7162              
  Branches     1503     1594      +91     
==========================================
+ Hits         3782     3839      +57     
- Misses       3075     3168      +93     
+ Partials      305      155     -150     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[tlylt]

I still think this approach is not the appropriate. The log is valid and intentional, it is printed because we intentionally used erroneous syntax. The fact that the log is printed is an assertion that the error is indeed detected.

I think a suitable solution to me is to print a notice when the test is executed (NOT the same as updating the production code to behavior differently in test mode). Something along the line of:

xxxx error test cases will be executed, we should expect the following warnings/errors be logged in console:
1. warning a: xxx
2. warning b: xxx

probably somewhere near

markbind/packages/cli/test/functional/test.js

Line 33 in 003822f

console.log(`Running ${siteName} tests`);

[AgentHagu]

I see, I think this solution should work. I can add something like this before the line you suggested, what do you think?

...
const expectedErrors = ['URLs are not allowed in the \'src\' attribute'];

console.log(`NOTE: The following ${expectedErrors.length} errors are expected to be thrown 
during the test run:`);
expectedErrors.forEach((error, index) => {
  console.log(`${index + 1}: ${error}`);
});

testSites.forEach((siteName) => {
...

Example screenshot:

An issue I foresee is that the note/warning gets hidden amongst the other messages so developers might not see that message. Would it be better to put it at the end? Let me know what you think, thanks!

[tlylt]

An issue I foresee is that the note/warning gets hidden amongst the other messages so developers might not see that message. Would it be better to put it at the end? Let me know what you think, thanks!

If u put it at the end, what happens when there's a test failure? Will it still be printed? I'm ok as long as the notice is printed, because if I was suspicious of the warning, I would have looked around for any clues, and at the front/back either is fine as long as it's mentioned

[AgentHagu]

I'm ok as long as the notice is printed, because if I was suspicious of the warning, I would have looked around for any clues, and at the front/back either is fine as long as it's mentioned

I see, in that case I'll just add it at the beginning since the tests stop after a test failure.

[AgentHagu]

I looked into NodeProcessor.test.ts and there they use a jest mock and spy of the logger.warn to check that its been called with a specific message. A similar implementation could be brought in to includePanelProcessor.test.ts to "hide" the core library's error messages being logged as mentioned in #2612.

For example, in includePanelProcessor.test.ts around line 286 we could add a spy on logger.error with something like this:

  const nodeProcessor = getNewDefaultNodeProcessor();
  const loggerErrorSpy = jest.spyOn(logger, 'error').mockImplementation(() => {});
  const result = await nodeProcessor.process(indexPath, index);

  const expected = [
    '<h1 id="index">Index</h1>',
    `<div style="color: red"><div style="color: red">${expectedErrorMessage}</div></div>`,
  ].join('\n');

  expect(result).toEqual(expected);
  expect(loggerErrorSpy).toHaveBeenCalledWith(new Error(expectedErrorMessage));
  loggerErrorSpy.mockRestore();

The log is valid and intentional, it is printed because we intentionally used erroneous syntax. The fact that the log is printed is an assertion that the error is indeed detected.

I think if we do it this way, we not only get to hide the error during testing which avoids confusion for developers but still also assert that the error has been detected and printed properly.

If we wish to still log the error to the console like in the CLI's case, we could also just choose to avoid a mock implementation but still check for the error message.

What do you think?

[tlylt]

I looked into NodeProcessor.test.ts and there they use a jest mock and spy of the logger.warn to check that its been called with a specific message. A similar implementation could be brought in to includePanelProcessor.test.ts to "hide" the core library's error messages being logged as mentioned in #2612.

is the error message in #2612 triggered by includePanelProcessor.test.ts? If not, I don't really get how mocking the logger helps the cli case.

The cli case invoke the build command via node xxx, i can't tell offhand whether you can mock out the logger in such cases (i doubt so).

If we wish to still log the error to the console like in the CLI's case, we could also just choose to avoid a mock implementation but still check for the error message.

Don't really get what you mean here. I do wish to still log the error to the console (retain the existing behavior), if we want to add assertions on the statements logged, i think that's a good to have. At the moment in these "integration" tests we are simply comparing the generated content via string compare.

[AgentHagu]

is the error message in #2612 triggered by includePanelProcessor.test.ts? If not, I don't really get how mocking the logger helps the cli case.

The cli case invoke the build command via node xxx, i can't tell offhand whether you can mock out the logger in such cases (i doubt so).

Ye, after looking into it, this doesn't help with the CLI case.

I do wish to still log the error to the console (retain the existing behavior), if we want to add assertions on the statements logged, i think that's a good to have. At the moment in these "integration" tests we are simply comparing the generated content via string compare.

Ok, I'll add the assertions to the logged statements.

[AgentHagu]

I've added the assertions and here are how the messages look now:

I've also update the loggers in includePanelProcessor.ts to only log the error message, rather than logging the entire error stack. This should improve user friendliness.

[AgentHagu]

I've added some originally missing tests for include and popover in includePanelProcessor.test.ts.

I've also updated the error message for empty src attribute for popover to say 'Empty src attribute in popover in: ${context.cwf}' rather than 'Empty src attribute in include in: ${context.cwf}'

[tlylt]

can we handle the plurality of errors just to make it nicer?

[AgentHagu]

Ok, will do!

[AgentHagu]

I've updated the code to now consider if the length of the expectedErrors is 1, let me know if any changes need to be made

[tlylt]

@AgentHagu thanks for the work done in this PR. I think it contains too many separate tasks and I would request that we split it into 2 or 3 PRs focusing on each item done

• This PR adds info messages for developers to inform them of the errors they should expect to see during npm run test.
• The PR also improves the error message shown.
• The PR also adds some missing tests for includePanelProcessor.ts

[AgentHagu]

@AgentHagu thanks for the work done in this PR. I think it contains too many separate tasks and I would request that we split it into 2 or 3 PRs focusing on each item done

Ok, will separate it as suggested!

[AgentHagu]

I've split the PR into #2702, #2703 and #2704 as suggested.

Additionally, #2702 is missing some info messages because those messages will be added when the other tests are added in #2704.