-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactor SyncAction tests to use real FwProjects pushing to local Mercurial server #347
Conversation
fb4cf7f
to
f8ffa8d
Compare
There's one more test, SynchronizeAction_CustomReferenceAtomicField_DoesNotThrowExceptionDuringSync, which could in theory be converted to use FwProjects instead of the old approach. However, it's essentially a regression test: there used to be a bug in FieldWorks 8.x that was causing certain custom fields to throw an exception when the FW value was converted to Mongo. There's workaround code in LfMerge to avoid that FW bug, and the test is verifying that with the workaround in place, those custom fields can be imported without error. Thing is, apparently the FW bug was fixed at some point between 8.x and 9.x, because when I removed the LfMerge workaround code, the test continued to pass. So either I wrote that test wrong in the first place (doubtful), or else the liblcm code fixed the bug sometime in the past few years. Which makes me a little cautious about making any changes to the regression test, as it was a rather complicated setup and since I can't make the test fail at the moment, I can't prove that any changes I make are still actually exercising the bug that used to be there. So I've decided not to touch the SynchronizeAction_CustomReferenceAtomicField_DoesNotThrowExceptionDuringSync test. We might remove it in the future, but for now let's leave it in place as a regression test, to ensure that the FW bug doesn't come back. |
f8ffa8d
to
d024820
Compare
One sample test that creates real FwProject, modifies a gloss, and pushes it to the mock LD server, setting up for real test code later.
d024820
to
6797c47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a couple comments. Something that I realized looking at the verification code. In these Synchronize tests we're only verifying against the mongo db. Is there another test class where we verify what changes were applied to the FW Project?
That way when you're reading the methods that call the shared code, you've already read the shared code.
…c-action-tests Need to change some method calls to match new API
Build was failing because it was building against the potential merge commit, where I had changed one API in the MongoConnectionDouble class. I hadn't noticed the API change because when I tested locally, I was running against the branch head, not the potential merge commit. So GitHub's default PR target just helped me NOT break the |
Fixes #346.
This PR converts all the SyncAction tests to use a real FwProject and call liblcm methods in order to edit it, rather than using pre-edited data in the testlangproj-modified project. This makes the tests easier to read (a LOT easier in some cases), as the changes expected in the FW project are visible in the test instead of being hidden away in non-code data. It also is a lot more flexible in terms of what kinds of future tests can be written, because it will be much simpler to load up a FW project and make whatever changes are needed.
One SyncAction test, testing a FW regression, was not converted; see #347 (comment) for the rationale. (Briefly, it's because I wouldn't be able to prove that the regression test was still testing the right thing).