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

Implement or remove pending tests #4803

Merged
merged 20 commits into from
Apr 7, 2022
Merged

Implement or remove pending tests #4803

merged 20 commits into from
Apr 7, 2022

Conversation

javierm
Copy link
Member

@javierm javierm commented Apr 4, 2022

Objectives

  • Allow commenting on polls as moderator/admin
  • Fix error messages when adding empty images to a poll question answer
  • Remove noise from the output of our test suite regarding pending tests
  • Implement tests for features that weren't tested
  • Remove references to features which aren't implemented

@javierm javierm added the Specs label Apr 4, 2022
@javierm javierm self-assigned this Apr 4, 2022
@javierm javierm added this to Reviewing in Consul Democracy via automation Apr 4, 2022
@javierm javierm moved this from Reviewing to Doing in Consul Democracy Apr 4, 2022
@javierm javierm force-pushed the pending_tests branch 5 times, most recently from ce1ab0e to e9d29b2 Compare April 4, 2022 12:56
@javierm javierm moved this from Doing to Reviewing in Consul Democracy Apr 4, 2022
@taitus taitus self-assigned this Apr 6, 2022
CONSUL doesn't implement blank votes via web; the comment was based on
the code used in Madrid, which was actually very complex.

And the concept of "all city" was also specific to Madrid. Poll
questions aren't associated to a geozone, so the geozone will depend on
the poll they're associated to.
The test "Sender email" already checks the receiver's name appears in
the copy sent to the sender.
@javierm javierm force-pushed the pending_tests branch 3 times, most recently from 492e9d1 to 8846c40 Compare April 7, 2022 11:34
Consul Democracy automation moved this from Reviewing to Testing Apr 7, 2022
@javierm javierm force-pushed the pending_tests branch 2 times, most recently from b5038b4 to e59da0b Compare April 7, 2022 11:55
@javierm javierm moved this from Testing to Reviewing in Consul Democracy Apr 7, 2022
The map feature was never implemented for debates (only for proposals
and budget investments) and it was crashing for debates because the page
didn't load the geozones. And we don't have a "geozone" field in the
debates form either.

So we're removing the map page alongside its (pending implementation)
tests.
This feature was actually implemented, but the test was checking the
wrong selectors.
So it works the same way as everywhere else.
We were skipping the test, but there's an identical test right below it.
This file only has tests related to tags; if the model doesn't have
tags, we simply wouldn't include `it_behaves_as` in their tests instead
of including it and then skipping it.
It looks like it was disabled because it was failing sometimes for some
reason. I haven't found the reason, though; we're changing the test a
little bit to make it easier to read. Enabling it will let us find out
whether it still fails.
It was disabled in commit 792b15b for unknown reasons.
They were marked as pending.

Note Capybara doesn't support finding a button by its `aria-labelledby`
attribute, so we're using the ugly `click_button "Yes"`, like we did in
commit fabe97e.
This feature was only enabled for proposals five years ago, and it
hasn't changed since then. The pending test only gets in the way.

Implement. Or implement not. There is no pending.
These tests are obsolete since commit 5ed308c.
We define the available locales in the test environment, so Spanish is
always available in this environment even if it isn't available in the
production environment.
This way the tests won't appear as "pending" when running the test
suite, and so we get rid of a lot of noise in the test results. There
doesn't seem to be a way to call `skip` without the test being marked as
"pending".

Note that in the globalizable tests we need to build a factory before
deciding whether an atribute is required or not (particularly for the
milestone factory, since milestone attributes are required depending on
the presence of other attributes). This isn't possible before we're
inside the test, so we can't add an `if:` condition to the test. So
we're adding the condition inside the test instead. A minor
inconvenience of this method is the test still runs even when the
condition is `false`.
We were finishing the test with the first "visit", so it was doing
nothing (other than potentially generating concurrency issues with other
tests).
I'd say this feature is actually tested in the "proposal polls specific
validations"; the empty test was probably added by accident in commit
4b8cc85.
Since we were creating a new answer in the form, we weren't getting the
errors associated to the answer the administrator was trying to create,
and so we were skipping the test.

Using the answer which contains the information about validation errors
fixes the issue and so we don't have to skip the tests.
The test is already working with poll question answers (which are the
only ones using `has_many_images`) as well.
Consul Democracy automation moved this from Reviewing to Testing Apr 7, 2022
@javierm javierm merged commit 3c5e168 into master Apr 7, 2022
Consul Democracy automation moved this from Testing to Release 1.5.0 Apr 7, 2022
@javierm javierm deleted the pending_tests branch April 7, 2022 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants