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

Don't import test code from main code #266

Closed
fvictorio opened this issue Oct 27, 2020 · 2 comments
Closed

Don't import test code from main code #266

fvictorio opened this issue Oct 27, 2020 · 2 comments
Assignees
Labels
chore good first issue Good candidate for newcomers :)

Comments

@fvictorio
Copy link
Contributor

Some file is importing something from some test file. This means that we have to include the test directory in the published package, but besides that it's just a bad idea. I haven't looked into it, but it's probably easy to fix.

@fvictorio fvictorio added chore good first issue Good candidate for newcomers :) labels Oct 27, 2020
@pfmescher pfmescher self-assigned this Feb 18, 2023
@pfmescher
Copy link

I haven't found any references to test files outside these

pablo@pop-os:~/protofire/solhint/lib$ grep 'require(.*test' . -r --include='*.js'
./rules/best-practises/reason-string.js:          code: require('../../../test/fixtures/best-practises/require-with-reason'),
./rules/best-practises/reason-string.js:          code: require('../../../test/fixtures/best-practises/require-without-reason'),
./rules/best-practises/max-states-count.js:          code: require('../../../test/fixtures/best-practises/number-of-states-low'),
./rules/best-practises/max-states-count.js:          code: require('../../../test/fixtures/best-practises/number-of-states-high'),
./rules/best-practises/payable-fallback.js:          code: require('../../../test/fixtures/best-practises/fallback-payable'),
./rules/best-practises/payable-fallback.js:          code: require('../../../test/fixtures/best-practises/fallback-not-payable'),
./rules/best-practises/code-complexity.js:          code: require('../../../test/fixtures/best-practises/code-complexity-low'),
./rules/best-practises/code-complexity.js:          code: require('../../../test/fixtures/best-practises/code-complexity-high'),
./rules/order/func-order.js:          code: require('../../../test/fixtures/order/func-order-constructor-first'),
./rules/order/func-order.js:          code: require('../../../test/fixtures/order/func-order-constructor-not-first'),
./rules/order/ordering.js:      good: require('../../../test/fixtures/order/ordering-correct'),
./rules/order/ordering.js:      bad: require('../../../test/fixtures/order/ordering-incorrect'),
./rules/order/visibility-modifier-order.js:          code: require('../../../test/fixtures/order/visibility-modifier-first'),
./rules/order/visibility-modifier-order.js:          code: require('../../../test/fixtures/order/visibility-modifier-not-first'),
./rules/security/reentrancy.js:          code: require('../../../test/fixtures/security/reentrancy-invulenarble')[0],
./rules/security/reentrancy.js:          code: require('../../../test/fixtures/security/reentrancy-invulenarble')[1],
./rules/security/reentrancy.js:          code: require('../../../test/fixtures/security/reentrancy-invulenarble')[2],
./rules/security/reentrancy.js:          code: require('../../../test/fixtures/security/reentrancy-vulenarble')[0],
./rules/security/reentrancy.js:          code: require('../../../test/fixtures/security/reentrancy-vulenarble')[1],
./rules/security/mark-callable-contracts.js:          code: require('../../../test/fixtures/security/external-contract-trusted'),
./rules/security/mark-callable-contracts.js:          code: require('../../../test/fixtures/security/external-contract-untrusted'),
./rules/security/func-visibility.js:          code: require('../../../test/fixtures/security/functions-with-visibility').join('\n'),
./rules/security/func-visibility.js:          code: require('../../../test/fixtures/security/functions-without-visibility').join('\n'),
./rules/security/check-send-result.js:          code: require('../../../test/fixtures/security/send-result-checked'),
./rules/security/check-send-result.js:          code: require('../../../test/fixtures/security/send-result-ignored'),
./rules/security/avoid-low-level-calls.js:const LOW_LEVEL_CALLS = require('../../../test/fixtures/security/low-level-calls')
./rules/miscellaneous/quotes.js:          code: require('../../../test/fixtures/miscellaneous/string-with-double-quotes'),
./rules/miscellaneous/quotes.js:          code: require('../../../test/fixtures/miscellaneous/string-with-single-quotes'),
./rules/miscellaneous/comprehensive-interface.js:          code: require('../../../test/fixtures/miscellaneous/public-function-with-override'),
./rules/miscellaneous/comprehensive-interface.js:          code: require('../../../test/fixtures/miscellaneous/public-function-no-override'),

which all seem to be related to documentation. I also noticed we are currently publishing the test folder in it's entirety since it is included in the files directive in the package.json file

  "files": [
    "/conf/",
    "/lib/",
    "/test/",
    "/solhint.js"
  ],

in any case, all the above examples are used as documentation and do not actually require anything else from any test files.. so I can propose two possible solutions to avoid shipping the test folder: move the fixtures to their own folder or explicitly include only the fixtures instead of the whole test folder (might require some workaround around the required paths for this to work)
in either case.. I don't really think this is a big deal at all and we can potentially just leave this as-is. What do you think? @fvictorio

@fvictorio
Copy link
Contributor Author

I'd say that we close this issue and open a new one about not publishing the test directory. I'm going to do that.

@fvictorio fvictorio closed this as not planned Won't fix, can't repro, duplicate, stale Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore good first issue Good candidate for newcomers :)
Projects
None yet
Development

No branches or pull requests

2 participants