Test cases for saveGraphJSON

[a1-su]

Proposed Changes

Added test cases for the saveGraphJSON function in app/Controllers/Graph.hs.

The test cases check the case with an empty graph/no inputted texts, shapes, or path nodes, the case with a single node, and the case with multiple connected nodes.

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)	X
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.
• I have updated the project Changelog (this is required for all changes).

After opening your pull request:

• I have verified that the CircleCI checks have passed.
• I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

In the getGraph function of app/Models/Graph.hs, it concatenates paths that are marked as a region and texts that do not intersect with any shape to the preexisting graphpaths and graphtexts lists, respectively, which already contain the added paths and texts. If you replace graphtexts ++ regionTexts with graphtexts and graphpaths ++ regions with graphpaths in the response object, the tests don't seem to fail, nor did I notice any obvious changes to the website.

courseography/app/Models/Graph.hs

Lines 47 to 58 in f5e8a3a

	(regions, _) = partition pathIsRegion graphpaths
	regionTexts = filter (not .
	intersectsWithShape (rects ++ ellipses))
	graphtexts
	response = object [
	("texts", toJSON $ graphtexts ++ regionTexts),
	("shapes", toJSON $ rects ++ ellipses),
	("paths", toJSON $ graphpaths ++ regions),
	("width", toJSON $ graphWidth $ entityVal graph),
	("height", toJSON $ graphHeight $ entityVal graph)
	]

To my understanding, we’re testing that the inputted JSON representing the graph is saved in the database properly, and we can get it from said database to compare with the original with getGraph from the app/Models/Graph.hs. However, the ++ regionTexts and ++ regions means that some text and paths are duplicated in the getGraph return value, compared to the original value. Since graphpaths already contains any paths in region, because the latter is a partition of the first, and similarly as graphtexts is already contained in regionTexts, the resulting concatenation contains duplicate elements.

I checked the blame and the commit from 10 years ago that added the code said “json-request: source and targets added,” so unfortunately I’m not really familiar with the context behind its addition. I think that since any text that intersects with a shape is automatically added to that shape's "text" attribute, the original intent of regionTexts might have been to only return the text that is not already combined into a shape.

Currently, I separated the input and expected JSON in the test cases and accounted for the duplicated nodes in the expected values, but I am unsure if this is the best approach for testing this function, especially considering that when testing if a JSON is saved, presumably the saved JSON should match the inputted JSON.

[coveralls]

Pull Request Test Coverage Report for Build f5d45b26-9bfa-494d-a9f2-81524192ee54

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.3%) to 55.461%

---

Totals
Change from base Build 6229c0e3-18e6-4f28-bb32-21717ad5183c:	0.3%
Covered Lines:	2239
Relevant Lines:	3966

---

💛 - Coveralls

[david-yz-liu]

Hi @a1-su, thanks for the detailed question. I believe you're correct: in this PR you can delete the ++ regionTexts and ++ regions from the getGraph function, and then simplify your test cases.

Please add a separate changelog entry for this change, and update the PR description as well. Normally I would ask you to make two separate PRs, but because this change is fairly small and will help simplify your test cases, I'm okay with you making it here.