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

Ensure that failed place startups don't block server startup unless the server is configured to do so #622

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

drivenflywheel
Copy link
Collaborator

@drivenflywheel drivenflywheel commented Nov 9, 2023

Mitigates potential NPEs introduced by #598. These NPEs can prevent server startup.

@drivenflywheel drivenflywheel added bug Something isn't working as intended priority This has mirrored work driving the need labels Nov 9, 2023
Copy link
Collaborator

@fbruton fbruton left a comment

Choose a reason for hiding this comment

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

Recommend creating a unit-test. Here is a test that will cause the current code in the baseline to throw an NPE. Feel free to incorporate and change to your liking.

    @Test
    void testInvisPlaceStartWithNullLocalPlace() throws IOException {
        // setup node, startup, and DirectoryPlace
        EmissaryNode node = new EmissaryNode();
        Startup startup = new Startup(node.getNodeConfigurator(), node);

        try (MockedStatic<DirectoryPlace> dirPlace = Mockito.mockStatic(DirectoryPlace.class)) {

            DirectoryEntry entry = mock(DirectoryEntry.class);
            when(entry.getLocalPlace()).thenReturn(null);

            List<DirectoryEntry> dirEntries = new ArrayList<>();
            dirEntries.add(0, entry);
            
            IDirectoryPlace directoryPlace = mock(IDirectoryPlace.class);
            when(directoryPlace.getEntries()).thenReturn(dirEntries);
            
            dirPlace.when(DirectoryPlace::lookup).thenReturn(directoryPlace);
            
            assertTrue(startup.verifyNoInvisiblePlacesStarted());
        }
        ```

sambish5
sambish5 previously approved these changes Nov 13, 2023
Copy link
Collaborator

@sambish5 sambish5 left a comment

Choose a reason for hiding this comment

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

Looks good to me, if you add the possible unit test I'll review again.

@drivenflywheel
Copy link
Collaborator Author

Unit test added - thanks, @fbruton!

Copy link
Collaborator

@sambish5 sambish5 left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

@jpdahlke jpdahlke merged commit 49899a8 into NationalSecurityAgency:master Nov 14, 2023
9 checks passed
@jpdahlke jpdahlke added this to the v8.0.0-M10 milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as intended priority This has mirrored work driving the need
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants