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

test: fix flaky test-fs-watch-encoding on OS X #7356

Closed
wants to merge 4 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 22, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test fs

Description of change

Depends on #7327. More info after that lands...

Copy of shell script used to stress test locally for the problem:

#!/bin/bash

BASE_PATH=./test/parallel
TEST=$1
COPIES=$2

for i in `seq 1 $COPIES`;
do
  cp $BASE_PATH/$TEST.js $BASE_PATH/$TEST-$i.js
done

runcount=1
while true; do
  echo "Run ${runcount}"
  /usr/bin/python tools/test.py --timeout=5 --mode=release parallel/${TEST}* -J
  if [ $? -ne 0 ]; then
    break
  fi
  ((runcount++))
done

Script (named stress.sh in the project root) instantiated with: bash stress.sh test-fs-watch-encoding 4

Without change in this PR, it fails after a handful of runs, usually less than 10 and never more than 100. With this change, it does not seem to fail at all.

@Trott Trott added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. macos Issues and PRs related to the macOS platform / OSX. labels Jun 22, 2016
@Trott
Copy link
Member Author

Trott commented Jun 22, 2016

R=@santigimeno

@Trott
Copy link
Member Author

Trott commented Jun 22, 2016

// around issues on OS X and SmartOS.
//
// On OS X, watch events are subject to peculiar timing oddities such that an
// event might fire our of order. The synchronous refreshing of the tmp
Copy link
Member

Choose a reason for hiding this comment

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

s/our/out/

@bnoordhuis
Copy link
Member

LGTM with typo.

@santigimeno
Copy link
Member

LGTM

const common = require('../common');
const fs = require('fs');
const path = require('path');
const assert = require('assert');

if (common.isFreeBSD) {
common.skip('Test currently not working on FreeBSD');
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this. The test now passes in FreeBSD because of the interval

@Trott
Copy link
Member Author

Trott commented Jun 22, 2016

Rebased, nits fixed, force pushed, new CI: https://ci.nodejs.org/job/node-test-pull-request/3051/

@Trott
Copy link
Member Author

Trott commented Jun 22, 2016

CI again: https://ci.nodejs.org/job/node-test-pull-request/3052/ Hooray, yellow, not red, I'll take it.

Trott added a commit to Trott/io.js that referenced this pull request Jun 24, 2016
PR-URL: nodejs#7356
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jun 24, 2016

Landed in 5267f29

@Trott Trott closed this Jun 24, 2016
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
PR-URL: #7356
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Jun 27, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
PR-URL: #7356
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
@MylesBorins
Copy link
Contributor

@Trott lts?

@Trott
Copy link
Member Author

Trott commented Jul 12, 2016

@thealphanerd No.

@Trott Trott deleted the kq branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants