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

Expanded automatic testing #26

Closed
henrypinkard opened this issue May 19, 2020 · 17 comments · Fixed by #458
Closed

Expanded automatic testing #26

henrypinkard opened this issue May 19, 2020 · 17 comments · Fixed by #458
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@henrypinkard
Copy link
Member

Incorporate some automatic testing framework (pytest?) to run all the examples, and add addition edge case examples of things one might do using Java-Python bridge.

Since almost all of these tests would need to run with micro-manager already running, it would probably be difficult to have github do this automatically, but it should be feasible to have a single script that can be run locally to do it

@henrypinkard henrypinkard added the enhancement New feature or request label May 19, 2020
@henrypinkard
Copy link
Member Author

Alternatively, could test just the core/ZMQserver parts by starting up a java process from the command line and attempting to run various test cases

@henrypinkard henrypinkard added the help wanted Extra attention is needed label Jul 30, 2020
@bryantChhun
Copy link

If you're ok with pytest (opposed to, say, unitttest), i'm going to take a stab at this today and tomorrow. First PR will be just a few simple examples and conftests.

Later we might want to translate some of your https://github.com/micro-manager/pycro-manager/tree/master/test to pytests.

@henrypinkard
Copy link
Member Author

Awesome! I'm not familiar with either, so I trust your judgement.

@ieivanov
Copy link
Collaborator

I would like to follow up on this. I think we can catch a lot of bugs if we can run pycromanager, AcqEngJ, NDViewer, etc. against the current build of MicroManager running the demo config. I think the pycromanager headless mode would really help here. @ziw-liu or @edyoshikun may be able to help from our side.

Any suggestions on what would be the best infrastructure and how to get started?

@henrypinkard
Copy link
Member Author

That would be awesome! And yes I totally agree.

AcqEngJ/NDTiff/pycro-manager should all be able to be tested in headless mode as you say. The rough steps would be:

  • Take the existing tests or some subset of them, which I've just been running manually on my own when things break, and convert them to a standardized form. That is, start each of them in headless mode and add assert statements to check for the presence of errors.
  • Package these up in some automated testing program. There are already a few automated tests (which I think use pytest) that live here. These can be used as a model. They only test the multi_d_acquisition_events function at present, since this doesn't require a micro-manager installation. You should be able to run the new tests locally with an installed nightly build
  • Once the tests work locally, they need to be run in a GitHub Action, so that they run every time a new PR is submitted. The current action that runs the MDA tests is here. Essentially, this action spins up a virtual machine in the cloud, does some installations on it, and runs the test script. The action will need to be modified so that the MM nightly build is installed from the command line (which is apparently possible)

Happy to offer more advice and answer questions, though I'm not super knowledgeable about the final step

For NDViewer, I'm unsure if the above will work, since I wouldn't expect the VMs to have graphics. Though I think this is less of a problem. I don't have any plans to add new features to it and it doesn't change very often. If there are future changes, I think there will just need to be some manual testing of pycromanager acquisitions. I probably should have done this before merging the breaking changes in micro-manager/NDViewer#16. @nicost FYI if you want to make further changes

@ieivanov
Copy link
Collaborator

I spoke with @ziw-liu and he will be excited to help set up a test environment, integrated with GitHub Actions, that will install MM and run a few basic integration tests at the start, as @henrypinkard suggested. From there, we can all help build out the test coverage.

@ziw-liu let us know if you run into troubles, and we'll all chip in with what we can.

@henrypinkard
Copy link
Member Author

Awesome!

@henrypinkard
Copy link
Member Author

And yeah I think once the framework is set up, the test coverage shouldn't be so hard to build bit by bit

@ieivanov
Copy link
Collaborator

What would be a good schedule for running these tests? On every PR? Or every night after the MM nightly build compiles?

Development of pycromanager is often coupled with development of AcqEngJ, NDTiffStorage, and NDViewer. Running pycromanager tests on every PR with the nightly build of MM will often fail as the nightly build does not yet incorporate changes to these three other libraries.

One fix for this would be to run GitHub actions every night after a new MM build is compiled.

Another option may be install the latest MM build, compile these three additional libraries (any idea how?), replace the out-of-date versions of the .jar files in the MM installation folder, and run PM tests with every PR.

I think running tests nightly is a good start. Once we have that, we can work towards the second solution I proposed. Does that make sense?

@henrypinkard
Copy link
Member Author

Ideally every PR, since this would prevent changes being pushed to pypi and broken versions being pip installable.

There are continuous integration builds that could be used, which are developer builds that occur every time something merges into micro-manager. If its no more work, probably best to install these over nightly builds.

But I don't think this alone would be enough, because ideally changes to AcqEngJ etc don't merge into MM until they've passed PM tests. Maven (the Java build system used by PM and its dependencies) can be used to build the PM java side, and it will automatically pull in the latest versions of these dependencies. Then it is just, as you say, a matter of copying them into the MM install and deleting whats already there.

To do this build, install maven (differs depending on your OS, but there should be straightforward instructions), you navigate into the pycro-manager/java directory and type mvn package in the terminal, which compiles PycroManagerJava.jar in pycro-manager/java/target. You can also use mvn dependency:copy-dependencies, which will download all required dependencies into pycro-manager/java/target/dependency

When getting ready to port this to github actions, this script might be a usefule reference. It executes a mvn deploy, which builds the jar and then uploads it to a public repositiory. It is useful because it shows the steps needed to get maven setup on a github action virtual machine.

@ziw-liu
Copy link

ziw-liu commented Nov 23, 2022

@henrypinkard Do you know of a way to enable the ZMQ server in MM after a fresh install without using the GUI? I've been trying to modify the Default User profile JSON file, but it seems like it will not be generated until the user registers in the GUI.

@henrypinkard
Copy link
Member Author

Hmm I'm not sure there is a way, though you don't need to do this if you're running in headless mode, so maybe this is a moot point?

@ziw-liu
Copy link

ziw-liu commented Nov 24, 2022

you don't need to do this if you're running in headless mode

I got the following error with this test. If PM does not need the MM user profile modification here, maybe it's due to port listening issues inside GitHub's runners.

_____________________________ test_start_headless _____________________________

    def test_start_headless():
        mm_app_path = r"C:\Program Files\Micro-Manager-2.0"
        config_file = os.path.join(mm_app_path, "MMConfig_demo.cfg")
>       start_headless(mm_app_path, config_file, timeout=10000)

pycromanager\test\test_acq_util.py:10: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
pycromanager\acq_util.py:94: in start_headless
    core = Core(timeout=timeout, port=port, **core_kwargs)
pycromanager\java_classes.py:84: in __new__
    bridge = Bridge(port=port, convert_camel_case=convert_camel_case, debug=debug, timeout=timeout)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pycromanager.bridge.Bridge object at 0x0000014807D9A280>, port = 4827
convert_camel_case = True, debug = False, ip_address = '[12](https://github.com/czbiohub/pycro-manager/actions/runs/3536864455/jobs/5936321051#step:8:13)7.0.0.1'
timeout = 10000, iterate = False

    def __init__(
        self, port: int=DEFAULT_PORT, convert_camel_case: bool=True,
            debug: bool=False, ip_address: str="127.0.0.1", timeout: int=DEFAULT_TIMEOUT, iterate: bool = False
    ):
        """
        Parameters
        ----------
        port : int
            The port on which the bridge operates
        convert_camel_case : bool
            If True, methods for Java objects that are passed across the bridge
            will have their names converted from camel case to underscores. i.e. class.methodName()
            becomes class.method_name()
        debug : bool
            If True print helpful stuff for debugging
        iterate : bool
            If True, ListArray will be iterated and give lists
        """
        self._weak_self_ref = weakref.ref(self)
        Bridge.local.bridges[port].append(self._weak_self_ref)
        if hasattr(self, '_ip_address'):
            return #already initialized
        self._ip_address = ip_address
        self._port = port
        self._convert_camel_case = convert_camel_case
        self._debug = debug
        self._timeout = timeout
        self._iterate = iterate
        self._main_socket = DataSocket(
            zmq.Context.instance(), port, zmq.REQ, debug=debug, ip_address=self._ip_address
        )
        self._main_socket.send({"command": "connect", "debug": debug})
        self._class_factory = _JavaClassFactory()
        reply_json = self._main_socket.receive(timeout=timeout)
        if reply_json is None:
>            raise TimeoutError(
                f"Socket timed out after {timeout} milliseconds. Is Micro-Manager running and is the ZMQ server on {port} option enabled?"
            )
E            TimeoutError: Socket timed out after 10000 milliseconds. Is Micro-Manager running and is the ZMQ server on 4827 option enabled?

pycromanager\bridge.py:275: TimeoutError

@henrypinkard
Copy link
Member Author

henrypinkard commented Nov 24, 2022

Yeah I think that error message is misleading. This checkbox is irrelevant in headless mode.

One strange thing I see:
ip_address = '[12](https://github.com/czbiohub/pycro-manager/actions/runs/3536864455/jobs/5936321051#step:8:13)7.0.0.1'

Which is weird because it doesn't seem like you've overriden the default ip address (which should be '127.0.0.1' -- localhost).

It also looks like you're using an older version. I would upgrade to latest version of PM and MM because a bunch of stuff has recently changed.

I agree githubs runners might be doing something weird, so I would recommend trying it on a machine that you actually have command line access to first.

Also try doing debug=True in start_headless. I recently added some better debugging that should give some output if the Java process runs into a problem

@ziw-liu
Copy link

ziw-liu commented Nov 27, 2022

One strange thing I see:
ip_address = '[12](https://github.com/czbiohub/pycro-manager/actions/runs/3536864455/jobs/5936321051#step:8:13)7.0.0.1'

Which is weird because it doesn't seem like you've overriden the default ip address (which should be '127.0.0.1' -- localhost).

This is due to a GitHub bug when copy-pasting GH Action logs. It tries to auto-detect job IDs as it does with issue/PR numbers and then replace with Markdown links. The actual log does say 127.0.0.1.

@ieivanov ieivanov mentioned this issue Nov 30, 2022
3 tasks
@ieivanov
Copy link
Collaborator

Managed to get a first working version. Will try the maven deployment next

@henrypinkard
Copy link
Member Author

Awesome work @ieivanov Just did the first successful run on #478

And I've added it as a branch protection rule, so future changes must pass the tests before merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants