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

Upgraded simpletest to v. 1.1.0 #5456

Closed
wants to merge 11 commits into from
Closed

Conversation

Srokap
Copy link
Contributor

@Srokap Srokap commented May 8, 2013

Main motivation was that old version failed a lot on strict error level (static calls in object context etc.).

Commented out usages of $this->swallowErrors(); as it's not defined in simpletest anymore.

I get now some errors when running tests:

Fail: D:/srokap/git/Srokap/Elgg/engine/tests/ElggCoreObjectTest.php -> ElggCoreObjectTest -> testElggObjectConstructorByGUID -> Identical expectation [Object: of ElggObjectTest] fails with [Object: of ElggObjectTest] with member [attributes] as key list [time_created, guid, type, subtype, owner_guid, container_guid, site_guid, access_id, time_updated, last_action, enabled, tables_split, tables_loaded, title, description] does not match key list [guid, tables_split, tables_loaded, type, subtype, owner_guid, site_guid, container_guid, access_id, time_created, time_updated, last_action, enabled, title, description] at [D:\srokap\git\Srokap\Elgg\engine\tests\ElggCoreObjectTest.php line 102]
Fail: D:/srokap/git/Srokap/Elgg/engine/tests/ElggCoreObjectTest.php -> ElggCoreObjectTest -> testElggObjectContainer -> Identical expectation [Object: of ElggGroup] fails with [Object: of ElggGroup] with member [attributes] as key list [time_created, guid, type, subtype, owner_guid, container_guid, site_guid, access_id, time_updated, last_action, enabled, tables_split, tables_loaded, name, description] does not match key list [guid, type, subtype, owner_guid, site_guid, container_guid, access_id, time_created, time_updated, last_action, enabled, tables_split, tables_loaded, name, description] at [D:\srokap\git\Srokap\Elgg\engine\tests\ElggCoreObjectTest.php line 154]
Fail: D:/srokap/git/Srokap/Elgg/engine/tests/ElggCoreUserTest.php -> ElggCoreUserTest -> testElggUserConstructorByGuid -> Identical expectation [Object: of ElggUser] fails with [Object: of ElggUser] with member [attributes] as key list [guid, tables_split, tables_loaded, type, subtype, owner_guid, site_guid, container_guid, access_id, time_created, time_updated, last_action, enabled, name, username, password, salt, email, language, code, banned, admin, prev_last_action, last_login, prev_last_login] does not match key list [guid, type, subtype, owner_guid, site_guid, container_guid, access_id, time_created, time_updated, last_action, enabled, tables_split, tables_loaded, name, username, password, salt, email, language, code, banned, admin, prev_last_action, last_login, prev_last_login] at [D:\srokap\git\Srokap\Elgg\engine\tests\ElggCoreUserTest.php line 97]
Fail: D:/srokap/git/Srokap/Elgg/engine/tests/ElggCoreUserTest.php -> ElggCoreUserTest -> testElggUserConstructorByDbRow -> Identical expectation [Object: of ElggUser] fails with [Object: of ElggUser] with member [attributes] as key list [guid, name, username, password, salt, email, language, code, banned, admin, last_action, prev_last_action, last_login, prev_last_login, tables_split, tables_loaded, type, subtype, owner_guid, site_guid, container_guid, access_id, time_created, time_updated, enabled] does not match key list [guid, type, subtype, owner_guid, site_guid, container_guid, access_id, time_created, time_updated, last_action, enabled, tables_split, tables_loaded, name, username, password, salt, email, language, code, banned, admin, prev_last_action, last_login, prev_last_login] at [D:\srokap\git\Srokap\Elgg\engine\tests\ElggCoreUserTest.php line 111]
Fail: D:/srokap/git/Srokap/Elgg/engine/tests/ElggCoreUserTest.php -> ElggCoreUserTest -> testElggUserConstructorByUsername -> Identical expectation [Object: of ElggUser] fails with [Object: of ElggUser] with member [attributes] as key list [time_created, guid, type, subtype, owner_guid, container_guid, site_guid, access_id, time_updated, last_action, enabled, tables_split, tables_loaded, name, username, password, salt, email, language, code, banned, admin, prev_last_action, last_login, prev_last_login] does not match key list [guid, type, subtype, owner_guid, site_guid, container_guid, access_id, time_created, time_updated, last_action, enabled, tables_split, tables_loaded, name, username, password, salt, email, language, code, banned, admin, prev_last_action, last_login, prev_last_login] at [D:\srokap\git\Srokap\Elgg\engine\tests\ElggCoreUserTest.php line 117]
Fail: D:\srokap\git\Srokap\Elgg\mod\web_services/tests/ElggCoreWebServicesApiTest.php -> ElggCoreWebServicesApiTest -> testApiAuthKeyBadKey -> Identical expectation [String: Missing API key] fails with [String: Bad API key] at character 0 with [Missing API key] and [Bad API key] at [D:\srokap\git\Srokap\Elgg\mod\web_services\tests\ElggCoreWebServicesApiTest.php line 309]

@Srokap
Copy link
Contributor Author

Srokap commented May 8, 2013

Ok, I've fixed problem with test for web_services, but remaining ones seem to show non deterministic order of attributes keys.

@cash
Copy link
Contributor

cash commented May 8, 2013

see #4106 and comments

@Srokap
Copy link
Contributor Author

Srokap commented May 8, 2013

Thanks. It seems that simpletest started to do deep check of the objects. I see two ways here:

  • having special comparison method for entities
  • ensuring deterministic order of the attributes array items

I like the latter, as It's more natural to work with, but may be hard to achieve when having additional internals that ie. are computed laizily

@Srokap
Copy link
Contributor Author

Srokap commented May 9, 2013

Ok, I've apparently fixed problem by not allowing to arbitrarly override attributes.

My previous tests were on 1.8.x install, on my current 1.9 i have these fails remaining:

Fail: G:/GIT/Srokap/Elgg1.9/engine/tests/ElggCoreEntityGetterFunctionsTest.php -> ElggCoreEntityGetterFunctionsTest -> testElggApiGettersEntitiesFromLocation -> Equal expectation fails because [Integer: 2] differs from [Integer: 3] by 1 at [G:\GIT\Srokap\Elgg1.9\engine\tests\ElggCoreEntityGetterFunctionsTest.php line 2540]
Fail: G:/GIT/Srokap/Elgg1.9/engine/tests/ElggCoreEntityGetterFunctionsTest.php -> ElggCoreEntityGetterFunctionsTest -> testElggApiGettersEntitiesFromLocation -> Expected true, got [Boolean: false] at [G:\GIT\Srokap\Elgg1.9\engine\tests\ElggCoreEntityGetterFunctionsTest.php line 2542]
Fail: G:/GIT/Srokap/Elgg1.9/engine/tests/ElggCoreObjectTest.php -> ElggCoreObjectTest -> testElggObjectConstructorByGUID -> Identical expectation [Object: of ElggObjectTest] fails with [Object: of ElggObjectTest] with member [attributes] with member [subtype] with type mismatch as [NULL] does not match [String: ] at [G:\GIT\Srokap\Elgg1.9\engine\tests\ElggCoreObjectTest.php line 102]
Fail: G:/GIT/Srokap/Elgg1.9/engine/tests/ElggCoreObjectTest.php -> ElggCoreObjectTest -> testElggObjectContainer -> Identical expectation [Object: of ElggGroup] fails with [Object: of ElggGroup] with member [attributes] with member [subtype] with type mismatch as [NULL] does not match [String: ] at [G:\GIT\Srokap\Elgg1.9\engine\tests\ElggCoreObjectTest.php line 154]
Fail: G:/GIT/Srokap/Elgg1.9/engine/tests/ElggCoreUserTest.php -> ElggCoreUserTest -> testElggUserConstructorByDbRow -> Identical expectation [Object: of ElggUser] fails with [Object: of ElggUser] with member [attributes] with member [last_action] at character 3 with [1364668685] and [1368060191] at [G:\GIT\Srokap\Elgg1.9\engine\tests\ElggCoreUserTest.php line 111]

@Srokap
Copy link
Contributor Author

Srokap commented May 9, 2013

I wonder if we shouldn't force empty subtypes to particular value. Right now we initialize it in the code as null, but fetch empty string from database. We probably could change it to null if it's not valid number.

@mrclay
Copy link
Member

mrclay commented May 9, 2013

Agreed. Ideally saving/reloading an entity should result in identical attribute values (except for those created during save of course).

@Srokap
Copy link
Contributor Author

Srokap commented May 9, 2013

I went through problems with types for subtypes, container_guid, owner_guid and access_id (see commit above).

Current problems are:

Fail: D:/srokap/git/Srokap/Elgg/engine/tests/ElggCoreObjectTest.php -> ElggCoreObjectTest -> testElggObjectConstructorByGUID -> Identical expectation [Object: of ElggObjectTest] fails with [Object: of ElggObjectTest] with member [attributes] with member [tables_loaded] because [Integer: 0] differs from [Integer: 2] by 2 at [D:\srokap\git\Srokap\Elgg\engine\tests\ElggCoreObjectTest.php line 102]
Fail: D:/srokap/git/Srokap/Elgg/engine/tests/ElggCoreObjectTest.php -> ElggCoreObjectTest -> testElggObjectContainer -> Identical expectation [Object: of ElggGroup] fails with [Object: of ElggGroup] with member [attributes] with member [tables_loaded] because [Integer: 0] differs from [Integer: 2] by 2 at [D:\srokap\git\Srokap\Elgg\engine\tests\ElggCoreObjectTest.php line 154]
Fail: D:/srokap/git/Srokap/Elgg/engine/tests/ElggCoreUserTest.php -> ElggCoreUserTest -> testElggUserConstructorByDbRow -> Identical expectation [Object: of ElggUser] fails with [Object: of ElggUser] with member [attributes] with member [last_action] at character 2 with [1339942107] and [1368126060] at [D:\srokap\git\Srokap\Elgg\engine\tests\ElggCoreUserTest.php line 111]

That's what I was affraid of. Having different value as tables_loaded sounds legitimate to me and I wouldn't like to tamper it for the sake of the tests. It seems that custom method for entity comparison is necessary after all.

I didn't dig into details of last_action time, but it may be also legitimate behaviour. Another argument for custom compare.

@Srokap
Copy link
Contributor Author

Srokap commented May 9, 2013

BTW, this issue refs: #4106, #4107, #4110, #4111

@Srokap
Copy link
Contributor Author

Srokap commented May 9, 2013

We should change timestamps (last_action etc.) type to int as well. I've tested difference in memory usage and it gives me ~20% more memory consumed for data stored in string instead of int.

See: https://gist.github.com/Srokap/5549917

@cash
Copy link
Contributor

cash commented May 9, 2013

See also #4136

@mrclay
Copy link
Member

mrclay commented May 10, 2013

@cash @brettp @ewinslow any reason we can't cast all the numeric attributes to ints? We should still keep nulls I think.

@cash
Copy link
Contributor

cash commented May 11, 2013

none that I can think of

@Srokap
Copy link
Contributor Author

Srokap commented May 12, 2013

Ok, it now passes all tests on my side.

I don't feel very well with last commit (i switch empty strings in name, description etc. to null in attribute value), as it's not elegant, but it's much simpler to have it this way than to change DB scheme and remove NOT NULL. Thoughts?

@Srokap
Copy link
Contributor Author

Srokap commented May 12, 2013

Just rebased whole branch on most recent master.

@cash
Copy link
Contributor

cash commented May 22, 2013

I rebased and then merged this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants