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

Bump version to 4.5 #27

Merged
merged 1 commit into from
Apr 12, 2019
Merged

Bump version to 4.5 #27

merged 1 commit into from
Apr 12, 2019

Conversation

csjall
Copy link
Contributor

@csjall csjall commented Apr 5, 2019

What does this Pull Request accomplish?

We are adding support for new hardware in the RSI Module repository. This breaks compatibility with existing system definition files. Mutate existing system definition files by rewriting the module model enum.

Why should this Pull Request be merged?

Customers will not be able to load existing system definition files without this change.

What testing has been done?

Load existing system definition file prior to version 4.5 and confirm properties are deserialized correctly.

We are adding support for new hardware in the RSI Module repository. This breaks compatibility with existing system definition files. Mutate existing system definition files by rewriting the module model enum.
@csjall csjall requested a review from rtzoeller as a code owner April 5, 2019 16:26
@rtzoeller
Copy link
Contributor

I will trigger a build and review once I can test the changes.

@niveristand-diff-bot
Copy link
Collaborator

Bleep bloop!

LabVIEW Diff Robot here with some diffs served up hot for your pull request.

Notice something funny? Help fix me on my GitHub repo.

Scan Engine Custom Device.lvlib--Get ECAT Config Wrapper.vi.png

capture

Scan Engine Custom Device.lvlib--Get Module Model.vi.png

capture

Scan Engine Custom Device.lvlib--Main - On Load.vi.png

capture

Scan Engine Custom Device.lvlib--Scan Engine Convert Module String.vi.png

capture

@niveristand-diff-bot
Copy link
Collaborator

Bleep bloop!

LabVIEW Diff Robot here with some diffs served up hot for your pull request.

Notice something funny? Help fix me on my GitHub repo.

Scan Engine Custom Device.lvlib--Get ECAT Config Wrapper.vi.png

capture

Scan Engine Custom Device.lvlib--Get Module Model.vi.png

capture

Scan Engine Custom Device.lvlib--Main - On Load.vi.png

capture

Scan Engine Custom Device.lvlib--Scan Engine Convert Module String.vi.png

capture

@rtzoeller
Copy link
Contributor

The tests for Specialty Digital are failing with these changes (tested in 2017, did not test the others). Curiously, if I open and save the test files such that the mutation changes are made the tests then pass. This would appear to point to the mutation not running (properly or at all?) when deployed directly through the API.

The code changes look good (aside from the test failures).

@csjall
Copy link
Contributor Author

csjall commented Apr 11, 2019

The tests for Specialty Digital are failing with these changes (tested in 2017, did not test the others). Curiously, if I open and save the test files such that the mutation changes are made the tests then pass. This would appear to point to the mutation not running (properly or at all?) when deployed directly through the API.

The code changes look good (aside from the test failures).

I will look into the mutation code and see why it is not running for the tests. I know I had to manually invoke the mutation for some unit tests, but in my mind the mutation should be running when we are calling through the API.

@buckd
Copy link
Collaborator

buckd commented Apr 11, 2019

The On Load action doesn't run during deployment (and I don't feel it should). Since the mutation is called in the On Load code and the tests only deploy existing sysdefs, no mutation will happen.

@rtzoeller
Copy link
Contributor

This problem is likely affecting other custom devices that do mutation. Once we have found a solution here, we should apply it to the other custom devices (e.g. the Engine Simulation Toolkit custom device).

@csjall
Copy link
Contributor Author

csjall commented Apr 11, 2019

OnLoad means "when the configuration front panel is loaded" not "when the system definition is loaded". Since we don't load the front panel for the custom device in the automated tests we never mutate the system definition. We could try to manually invoke the OnLoad action as part of the deployment in the automated testing tools.

I have created an issue for this.

Copy link
Contributor

@rtzoeller rtzoeller left a comment

Choose a reason for hiding this comment

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

Since we have multiple pull requests in flight and the known set of issues have been filed, let's merge this.

Either fix the tests or skip them.

@csjall csjall merged commit 4b72ab9 into master Apr 12, 2019
@csjall csjall deleted the dev/versionbump branch April 12, 2019 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants