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

Fixed tests to work with windows file separator too using OS agnostic… #23

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

Conversation

stokescomp
Copy link

… DIRECTORY_SEPARATOR

@majkel89 majkel89 changed the title Fixed tests to work with windows file separator too using OS agnostic… #22 Fixed tests to work with windows file separator too using OS agnostic… May 21, 2019
@majkel89 majkel89 changed the title #22 Fixed tests to work with windows file separator too using OS agnostic… Fixed tests to work with windows file separator too using OS agnostic… May 21, 2019
@@ -26,13 +26,13 @@ class ExtCaseInsensitiveGeneratorTest extends TestBase
public function testGenerator()
{
$extensions = array();
$generator = new ExtCaseInsensitiveGenerator('some/FILE.php');
$generator = new ExtCaseInsensitiveGenerator('some'.DIRECTORY_SEPARATOR.'FILE.php');
Copy link
Owner

Choose a reason for hiding this comment

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

I would leave the paths as it was before. The constant decreases readability

Copy link
Author

Choose a reason for hiding this comment

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

I will revert these.

@@ -142,15 +143,15 @@ public function setLength($length) {
}

/**
* Returns filed decimalCount
* Returns field decimalCount
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for pointing out the typos

array('123', 123.0),
array(123, 123.0),
array(' 2', 2.0),
array('1234.1234', 123.0),
Copy link
Owner

Choose a reason for hiding this comment

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

strange that in this test the fraction part is stripped

@majkel89
Copy link
Owner

Please fix the tests

* @throws Exception
*/
public function testReadDbase4() {
$dbf = Table::fromFile('tests/fixtures/dBase4.dbf');
Copy link
Owner

Choose a reason for hiding this comment

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

This test fails because it cannot find corresponding memo file (fbt)

Copy link
Author

Choose a reason for hiding this comment

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

is that FPT format? How do you choose one over the other?
In readme I see:

Supported memo formats
  • DBT
  • FPT

In the failed test I see:

  1. org\majkel\dbase\DBase4Test::testReadDbase4
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
    -memo1
    +▒▒
    +memo1

$header = $this->reflect($format)->createHeader($this->getHeaderData(array(
'v' => 666,
)));
self::assertTrue($header->isValid());
Copy link
Owner

Choose a reason for hiding this comment

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

Please investigate why this test fails. The only case where the header could be invalid is when the calculated records count does not match file size

Copy link
Author

Choose a reason for hiding this comment

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

I see what is going on here.
It is using bitwise logic here.
return $header->setValid($header->getVersion() & $this->getVersion());
So with dBase4 I need to use 668 for the test since it will be doing 668 & 4 in the above setValid method. And 4 in binary is 100. I have to choose a number that has its bit flipped in the 3rd spot and 666 doesn't do this but 668 does.
668 is ‭0010 1001 1100‬ in binary.

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.

3 participants