Skip to content
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

Handle generated plantuml files in functional testcases #1993

Merged
merged 7 commits into from
Aug 11, 2022

Conversation

tlylt
Copy link
Contributor

@tlylt tlylt commented Aug 7, 2022

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:

Overview of changes:
Fixes #1937
Fixes #1130 (arguably, this is an "expedient" solution such that if we include more/adjust plantuml files in our test sites in the future, we will need to update the handling code accordingly to ignore them. Let me know if I should change the keyword here to leave that issue open)

Also:

  • update a warning in our DG -> workflows to mention the caveat, just in case
  • add a note about how to update plantuml files that are ignored

Anything you'd like to highlight/discuss:
Besides deleting the plantuml images and git ignore them, I realized I will also need to delete them from the expected folder again, before running the functional test. This is because these files which will still be generated after an npm run updatetest will affect testing.
The plantuml images will still be generated in the expected folder, in case devs want to look at them.

Wonder if we should do the same thing for the font files, as they appear to be problematic as well.

  • at the moment: no

Testing instructions:
For docs update:
https://deploy-preview-1993--markbind-master.netlify.app/devguide/workflow#updating-and-writing-tests
image
image

For testing code:

  • npm run test should succeed
  • Running npm run updatetest should no longer show plantuml files as uncommitted changes, but those files should actually be generated in the expected folder (because they are added to .gitignore)
  • Re running npm run test should pass again, and those generated plantuml files should still appear in the expected folder

Proposed commit message: (wrap lines at 72 characters)
Handle generated plantuml files in test sites

Unrelated changes to png files (generated by plantuml) appear
when updating functional test cases.

Let's mention the need to ignore .png files (and binary files in general)
in our developer guide. Let's also update functional tests such that
some files can be ignored, to reduce the need to deal with
unrelated changes.

This helps newcomers to know how to update functional tests
properly and reduce unnecessary dev work to maintain the 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

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Wonder if we should do the same thing for the font files, as they appear to be problematic as well.

Some of the fonts need to be updated together with the dependency (Bumping font-awesome etc). Not sure about the rest but the fonts on my end don't really change during updatetest (Happened only a few times as far as I can remember). If this is a rare issue, we can do the manual reminder method as it seems more complicated to maintain a long list of ignored fonts.

packages/cli/test/functional/testUtil/compare.js Outdated Show resolved Hide resolved
packages/cli/test/functional/testSites.js Show resolved Hide resolved
Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Nice job @tlylt!

LGTM 👍

@jonahtanjz jonahtanjz added this to the 4.0.1 milestone Aug 10, 2022
@jonahtanjz jonahtanjz merged commit da0cd19 into MarkBind:master Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants