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

WIP: Add hasProperty() assertion #46

Closed

Conversation

seansellek
Copy link

@seansellek seansellek commented Jan 12, 2018

This takes the first steps towards addressing #38 by adding a base .hasProperty() assertion as mentioned by @Turbo87.

The assertion supports providing a custom message to facilitate building shortcuts such as .isChecked().

Resolves #55

@seansellek
Copy link
Author

seansellek commented Jan 12, 2018

There seems to be a permissions issue surrounding Chrome in CI 😥

You need to make sure that /opt/google/chrome/chrome-sandbox is owned by root and has mode 4755

Update: It's a known issue with Travis CI travis-ci/travis-ci#8836

A couple workarounds:

  1. Use sudo
# travis.yml
sudo: required
addons:
 chrome: stable
  1. use Firefox:
# travis.yml
addons:
    firefox: 57.0

thoughts?

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 13, 2018

@seansellek I'm currently on vacation, I'll have a look once I'm back (7 days)

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 30, 2018

@seansellek 👋 we've just merged #48, which should fix the Chrome issue. Can you rebase this PR?

.gitignore Outdated
@@ -16,6 +16,7 @@
npm-debug.log*
yarn-error.log
testem.log
.vscode/
Copy link
Collaborator

Choose a reason for hiding this comment

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

This stuff should be move to your global .gitignore file (see https://help.github.com/articles/ignoring-files/)

Only project specific files should be added to this file, but no individual editor config files etc.

@seansellek
Copy link
Author

@Turbo87 sorry it took a bit for me to get back to this. Rebased, removed the .gitignore commit, and fixed a bug in the tests, getting coverage up to 100%.

@seansellek seansellek force-pushed the has-property-assertion branch 2 times, most recently from c0961ec to 4215911 Compare February 10, 2018 22:30
* if the property `value` matches the provided `value`.
*
* @param {string} property
* @param {object?} options
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we should mimic the hasAttribute API instead which accepts name, value?, message? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 to this. There's a lot to be said for the API being consistent for like methods/functions.

@Turbo87 Turbo87 changed the title Adding hasProperty() assertion Add hasProperty() assertion Mar 11, 2018
**Examples**

```javascript
assert.dom('input#agreedToTOS').hasProperty('checked', { value: true });
Copy link
Collaborator

Choose a reason for hiding this comment

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

as mentioned before, I think this should mimic the API that we have for hasAttribute()

@scalvert
Copy link
Collaborator

scalvert commented Sep 6, 2018

Thanks for pinging on this, @Turbo87. I've been pushing on it over the last little while, but haven't had as much time to get it through.

I've aligned it more to the hasAttribute signature, and am just working through the tests. I'll try to get this finished up over the next little while.

@Turbo87
Copy link
Collaborator

Turbo87 commented Sep 6, 2018

sweet, thanks for working on it :)

@Turbo87 Turbo87 changed the title Add hasProperty() assertion WIP: Add hasProperty() assertion Nov 29, 2018
@Turbo87
Copy link
Collaborator

Turbo87 commented Jun 27, 2019

closing due to inactivity

@Turbo87 Turbo87 closed this Jun 27, 2019
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.

Add hasProperty() assertion
3 participants