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

Tests on windows #83

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SeverinLeonhardt
Copy link

As promised in #81 I've looked into running the tests on Windows.

Only c3c6000 is strictly necessary. The other two commits are just something easy to fix.

It's best practice to not modify the source during a build so a single
source folder can be used to build multiple variations. Especially with
WSL on Windows this makes it very easy to verify code changes work even
across platforms.
Building the unit tests on Windows failed at the linking stage:

	LINK : fatal error LNK1104: cannot open file 'scylla-cpp-driver_static.lib'

This is caused by the tests being forced to use the static library on
Windows but link against `${PROJECT_LIB_NAME_TARGET}` which is set to
`scylla-cpp-driver_static` in the main `CMakeLists.txt` while the target
was actually still called `cassandra_static`.

Renamed the static library target to be consistent with the dynamic
library target and `${PROJECT_LIB_NAME_TARGET}` so the tests can be
successfully linked on Windows.
The test failed with:

	tests/src/unit/tests/test_protocol_version.cpp:33: Failure
	      Expected: ProtocolVersion(CASS_PROTOCOL_VERSION_DSEV2)
	      Which is: 4-byte object <42-00 00-00>
	To be equal to: ProtocolVersion::highest_supported()
	      Which is: 4-byte object <04-00 00-00>

This most likely broke with 9d37d1b
where the default value of the parameter changed.

Changed the expected value to match what the function now returns since
the mentioned commit looks like it intentionally changed that value.
Copy link

@Gor027 Gor027 left a comment

Choose a reason for hiding this comment

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

LGTM.

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.

2 participants