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

Fix issues with grouped tests #1107

Merged
merged 19 commits into from
Feb 17, 2022
Merged

Fix issues with grouped tests #1107

merged 19 commits into from
Feb 17, 2022

Conversation

dellaert
Copy link
Member

Fixes #91 and hopefully addresses some issues in #1102

@dellaert
Copy link
Member Author

Hey @mikesheffler, this works now. If you review I can merge, and then you can merge in develop to see the effect on Windows.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

All we had to do was add namespace guards to all the global variables in the tests??? Sheesh, I'm kinda sad that #91 took this long to resolve. LGTM!!!

EXPECT(serializationTestHelpers::equalsObj(factor));
EXPECT(serializationTestHelpers::equalsXML(factor));
EXPECT(serializationTestHelpers::equalsBinary(factor));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason we've killed these two tests?

// Export Noisemodels
// See http://www.boost.org/doc/libs/1_32_0/libs/serialization/doc/special.html
BOOST_CLASS_EXPORT(gtsam::noiseModel::Isotropic);
BOOST_CLASS_EXPORT(gtsam::noiseModel::Unit);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah nvm my previous comment. I saw additional serialization tests being removed and then realized I should scroll down a bit.

@varunagrawal varunagrawal merged commit 2b78b96 into develop Feb 17, 2022
@varunagrawal varunagrawal deleted the fix/91_single_test_exe branch February 17, 2022 05:12
@mikesheffler
Copy link
Contributor

Hey @mikesheffler, this works now. If you review I can merge, and then you can merge in develop to see the effect on Windows.

Sorry, I've been tied up all day.

In plain English, what's the goal here? Are we trying to get all of the unit tests working in this build, or just a select subset? I'm fine either way, but if the goal is to get everything going, I wonder if there might be some remaining issues. I pulled and then tried building everything, but I'm still seeing a bunch of build errors. I swear that this isn't sarcastic, but: Am I doing something wrong?

I'm ...

  1. Running CMake like I always do
  2. Opening the GTSAM.sln that pops out
  3. Clicking on the little triangle next to Unit tests in Solution Explorer
  4. Right-click on check.geometry
  5. Selecting Build
  6. Seeing a bunch of errors in the console

That process does work for e.g. check.base

I thought maybe the GUI was hiding what I wanted, so I ...

  1. Opened the x64 Native Tools Command Prompt for VS 2019
  2. cded to my GTSAM build directory
  3. Used a spell similar to this workflow file: cmake --build . -j 4 --config Debug --target check.geometry
  4. Saw a bunch of errors in the console

Like the GUI-based approach, this does work for check.base.

I saw in one of the commits above that @dellaert 's commit message was fix check.sam, but that's one of the ones that I can't build. This is at 55ad184, btw -- I'm at the current HEAD of develop.

@dellaert
Copy link
Member Author

dellaert commented Feb 17, 2022

Sorry, I've been tied up all day.

I get that :-)

In plain English, what's the goal here?

Trying to resolve #91 and #1087, ultimately getting MATLAB and python wrappers working on windows. Borrowing from your little bird analogy, I propose using make check as the canary in the coal mine: this would suss out all compile and linking errors in gtsam proper, make our windows CI better, and move on to any issues in building the MATLAB wrapper. Makes sense?

No, I don't think you're doing anything wrong if it works with base. What are the errors with check.geometry? Maybe paste them in #1087 , as we now closed this PR. I'll repeat this comment there.

@varunagrawal
Copy link
Collaborator

My $0.02: I made some great progress on fixing most of the dllexport issues, and only have geometry, nonlinear, sam and slam not passing.

Except for geometry, every other failure is test failure which should be easier to debug than the dllexport issues. I'll create a PR so y'all can take a look.

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.

GTSAM_SINGLE_TEST_EXE option causes compile errors
3 participants